Implement a freeze: unpacker option #194
Conversation
casperisfine
force-pushed
the
global-freeze-option
branch
from
6f02d11 to
ee63e2e
Compare
|
I went ahead and implemented the jruby version. |
casperisfine
force-pushed
the
global-freeze-option
branch
from
ee63e2e to
b730ed4
Compare
| return runtime.newString(byteList); | ||
| RubyString string = runtime.newString(byteList); | ||
| if (this.freeze) { | ||
| string.setFrozen(true); |
There was a problem hiding this comment.
Ruby#freezeAndDedupString looks to freeze&dedup the string, not only dedup.
Is there any reason to call setFrozen(true) explicitly here?
There was a problem hiding this comment.
Yes, today it's a bit useless, however in theory if you call it with a frozen string, it could directly intern that string without duplicating it first. That's what MRI 3.0 now does, see: ruby/spec#788
I do hope to change that behavior in JRuby at some point, so I figured we might as well call it with a frozen string directly.
casperisfine
force-pushed
the
global-freeze-option
branch
from
b730ed4 to
f3f5168
Compare
|
Plz ping me when you solve the problem (and failing CI). |
casperisfine
force-pushed
the
global-freeze-option
branch
from
f3f5168 to
d546a15
Compare
If set to true all parsed objects will be immediately frozen, and strings will be deduplicated if the Ruby implementation allows it.
casperisfine
force-pushed
the
global-freeze-option
branch
from
d546a15 to
4bb65fa
Compare
It should be good now. CI is green. Sorry for the last change. |
|
I'll merge this change tomorrow if there's no further commits/comments. |
|
Works for me, thanks! |
As discussed in #190
If set to true all parsed objects will be immediately frozen, and strings will be deduplicated if the Ruby implementation allows it.
This feature is meant to be similar to Pysch's
freeze:option: ruby/psych#414For now this option is only useful if you plan to keep the parsed objects around, and want to reduce their memory footprint.
But hopefully Ruby 3.0 should soon expose
rb_fstring_new(const char *ptr, long len)(likely under another name) which should allow to reduce allocations by directly looking up existing strings without allocating a new one.cc @tagomoris
NB: I presume that a java implementation is needed, I'm not too well versed into Java by I can try my hand at it. However I'd rather have your feedback on the MRI implementation first.