Skip to content

Memory optimizations #90

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Memory optimizations #90

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions lib/redis_client/ruby_connection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ def initialize(config, connect_timeout:, read_timeout:, write_timeout:)
read_timeout: read_timeout,
write_timeout: write_timeout,
)

@buffer = String.new(encoding: Encoding::BINARY, capacity: 127)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That 127 is quite arbitrary, would be worth putting some thoughts into it, or making it configurable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was taken from the buffer definition it already uses -

redis-client/lib/redis_client/ruby_connection/resp3.rb

Lines 53 to 55 in aa5a308

def new_buffer
String.new(encoding: Encoding::BINARY, capacity: 127)
end

rescue SystemCallError, OpenSSL::SSL::SSLError, SocketError => error
raise CannotConnectError, error.message, error.backtrace
end
Expand All @@ -98,21 +100,22 @@ def write_timeout=(timeout)
end

def write(command)
buffer = RESP3.dump(command)
@buffer.clear
RESP3.dump(command, @buffer)
Comment on lines +103 to +104
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So String#clear here free the malloced region: https://bugs.ruby-lang.org/issues/17790.

So this save allocating a string slot, but will malloc anyway which will eventually trigger a GC.

If we want to be smart we want to re-used that malloced region, but have to be careful not to end up with a giant buffer, so we'd need to clear it if it past a certain size. Unfortunately the only way to get the size of the malloc is via ObjectSpace.memsize_of(str). I'd need to check its performance first.

We also have to be super careful not to leak data, as that happened in the past with a similar optimization ruby/net-protocol#19

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to be smart we want to re-used that malloced region

Can we currently do this from ruby? I see the proposed patch into ruby is not merged yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we currently do this from ruby?

Well, if you pass the string to read without clearing it, it will re-use that space.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we are using it just for write in this case (write and write_multi), so this is not yet possible?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, sorry I missed we where passing it to RESP3.dump. Yeah I see no solution here.

I think if we call clear we start over from an empty string, so we lose the pre-allocation benefits.

begin
@io.write(buffer)
@io.write(@buffer)
rescue SystemCallError, IOError => error
raise ConnectionError, error.message
end
end

def write_multi(commands)
buffer = nil
@buffer.clear
commands.each do |command|
buffer = RESP3.dump(command, buffer)
RESP3.dump(command, @buffer)
end
begin
@io.write(buffer)
@io.write(@buffer)
rescue SystemCallError, IOError => error
raise ConnectionError, error.message
end
Expand Down