OP_COMPRESSION #160
OP_COMPRESSION #160
Conversation
xdg
left a comment
xdg
left a comment
There was a problem hiding this comment.
Solid work! Thanks! I don't think it will take much to address my comments.
| =attr zlib_compression_level | ||
| An integer from C<-1> to C<9> specifying the compression level to use | ||
| when L</compression> is set to C<zlib>. |
There was a problem hiding this comment.
Please note the default (and meaning) of -1.
| ( ref( $self->ssl ) eq 'HASH' ? ( SSL_options => $self->ssl ) : () ), | ||
| }, | ||
| monitoring_callback => $self->monitoring_callback, | ||
| compression => $self->compressors, |
There was a problem hiding this comment.
I think this attribute should be called "compressors" for consistency.
|
|
||
| my %write_opt; | ||
| my $command_name = do { | ||
| my $command = _to_tied_ixhash($self->{query}); |
There was a problem hiding this comment.
I'm a little concerned that this might be an expensive conversion to do for each operation just to get the command name. Extracting the _id field in insertion is a similar problem (where you know the document is one of only a few allowed types) and you can see how we do that here: https://github.com/mongodb/mongo-perl-driver/blob/880e42f6935fcac785b8d839a94d867a4f605066/lib/MongoDB/Role/_InsertPreEncoder.pm#L42-L50
| return $res; | ||
| } | ||
|
|
||
| sub is_compressible { |
There was a problem hiding this comment.
This is way too expensive for every operation (sub call and grep over list). Please just create a file-scoped hash with the prohibited command keys and check like $write_opt{disable_compression} = $IS_NOT_COMPRESSIBLE{$command_name}
|
|
||
| sub write { | ||
| my ( $self, $buf ) = @_; | ||
| my ( $self, $buf, %write_opt ) = @_; |
There was a problem hiding this comment.
Please make the option into a hashref. In the driver we try to avoid passing slurpy args.
| sub try_uncompress { | ||
| my ($msg) = @_; | ||
|
|
||
| my ($len, $request_id, $response_to, $op_code, $orig_op_code, undef, $comp_id) |
There was a problem hiding this comment.
You should unpack the original msg length here and use it below rather than the computed length of the reconstituted message. If there's any corruption in the decompression, that could help detect the problem.
|
|
||
| $msg = substr $msg, P_COMPRESSED_PREFIX_LENGTH; | ||
|
|
||
| for my $comp_name (keys %COMPRESSOR) { |
There was a problem hiding this comment.
Rather than looping (O(N)), just build a file-scoped decompressor array that you can index into:
my @DECOMPRESSORS = [ sub { shift }, undef, \&_decompress_zlib ];
There was a problem hiding this comment.
And if the compressor becomes a coderef attribute of server, then you don't need the %COMPRESSOR hash at all, only an array of decompressor coderefs.
| if ($COMPRESSOR{$comp_name}[0] == $comp_id) { | ||
| my $decomp_msg = $COMPRESSOR{$comp_name}[2]->($msg); | ||
| my $done = | ||
| pack(P_HEADER, 0, $request_id, $response_to, $orig_op_code) |
There was a problem hiding this comment.
Here, instead of 0, pack the original length extracted from the compressed header.
| default => sub { {} }, | ||
| ); | ||
|
|
||
| has compressor => ( |
There was a problem hiding this comment.
As discussed above, maybe this should become an optional coderef that encapsulates the zlib_compression_level. The closure generator could still live in _Protocol so all the compression stuff is in one place and the _build_compressor method just calls MongoDB::_Protocol::get_compressor($name, $opts) or something like that.
|
|
||
| sub _build_compressor { | ||
| my ($self) = @_; | ||
| my ($comp) = @{ ($self->is_master || {})->{compression} || [] }; |
There was a problem hiding this comment.
A subtlety here: I don't think the server is required to return compressors in the preference order sent by the client, only an intersection. The client could send "zlib, snappy" and the server could reply with "snappy, zlib". So server probably needs to get a "preferred_compressors" attribute and this should take the servers reply in preference order.
|
Fixed and rebased version available in PR #162. |
3 participants
This adds:
Compress::Zlib.compressorsandzlibCompressionLevelin constructor and URIs.devel/t-dynamic/OP_COMPRESSION.t.PERL_MONGO_TEST_COMPRESSION=zlibto run the test suite with compression enabled.