[GitHub] [lucene] luyuncheng commented on pull request #987: LUCENE-10627: Using CompositeByteBuf to Reduce Memory Copy
luyuncheng commented on PR #987: URL: https://github.com/apache/lucene/pull/987#issuecomment-1186947659 > Thanks for the iteration, the high-level idea looks good to me, I left some suggestions. @jpountz Thanks for your nice suggestions. at latest commits [ebcbdd](https://github.com/apache/lucene/commit/457bab5abfbaa3aabebe99a97e645b4f623be15f) 1. change statement byteBuffers in Lucene90CompressingStoredFieldsWriter 2. move sliceOne logic into ByteBuffersDataInput level and it would determine whether use internal buffers or create a new buffer 3. delete DeflateWithPresetDictCompressionMode#doCompress method which param is byte[] 4. delete copyOneDoc-> readBytes which instance ByteArrayDataInput optimize. and i would create a new issue to reduce memory copy in copyOneDoc -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] luyuncheng commented on pull request #987: LUCENE-10627: Using CompositeByteBuf to Reduce Memory Copy
luyuncheng commented on PR #987: URL: https://github.com/apache/lucene/pull/987#issuecomment-1183266339 > we prefer to fork the code so that old codecs still rely on the unchanged code (which should move to lucene/backward-codecs) Thanks for the your advice @jpountz , i think it is LGTM. At commit [448e25](https://github.com/luyuncheng/lucene/commit/448e254e1d3c5323f369236492de0d512f537ac2) i try to move old compressor into backward_codecs. And we only use one method `compress(ByteBuffersDataInput buffersInput, DataOutput out)` in Compressor When using ByteBuffersDataInput in compress mehtod, it can 1. Reuse ByteBuffersDataInput reduce memory copy in stored fields compressing 2. Reuse ByteBuffersDataInput reduce memory copy in TermVectors compressing 3. Reuse ByteArrayDataInput reduce memory copy in copyOneDoc -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] luyuncheng commented on pull request #987: LUCENE-10627: Using CompositeByteBuf to Reduce Memory Copy
luyuncheng commented on PR #987: URL: https://github.com/apache/lucene/pull/987#issuecomment-1181632413 > Would it be possible to remove all `CompressionMode#compress` variants that take a `byte[]` now that you introduced a new method that takes a `ByteBuffersDataInput`? > > Also maybe we should keep old codecs unmodified and only make this change to `Lucene90Codec` where it makes most sense? Hi @jpountz Thanks for reviewing this code. I prefer keeping old codecs unmodified, because `CompressionMode#compress` is a public abstract method, if we change it with variants `ByteBuffersDataInput` we need to backport in many codecs, like [commits](https://github.com/apache/lucene/blob/382962f22df3ee3af3fb538b877c98d61a622ddb/lucene/backward-codecs/src/java/org/apache/lucene/backward_codecs/lucene50/Lucene50StoredFieldsFormat.java). And if we only using compress method with variants ByteBuffersDataInput in LUCENE90, we can not using abstract method `Compressor.compress`, when we want to use other compression mode. Would it be possible to add a new method in Compressor, like following? it can keep the old codecs unmodified, and method with variants ByteBuffersDataInput only can be called in Lucene90Codec. ``` public abstract void compress(byte[] bytes, int off, int len, DataOutput out) throws IOException; public void compress(CompositeByteBuf compositeByteBuf, DataOutput out) throws IOException; ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
