Skip to content

Copy and embed for serialization format#3972

Draft
kddnewton wants to merge 1 commit intomainfrom
copy-and-embed
Draft

Copy and embed for serialization format#3972
kddnewton wants to merge 1 commit intomainfrom
copy-and-embed

Conversation

@kddnewton
Copy link
Collaborator

Fixes #1537

@eregon @enebo would you both be able to experiment with this branch and tell me what you think? If it works better for JRuby/TruffleRuby I'm happy to just merge this as the default and remove the branching logic inside the deserializers

uint32_t owned_mask = 1U << 31;
// Write the constant contents into the buffer after the constant
// pool. In place of the source offset, we store a buffer offset
// with the high bit set to indicate embedded content.
Copy link
Member

@eregon eregon Mar 7, 2026

Choose a reason for hiding this comment

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

Since they would all be embedded we wouldn't need the high bit anymore (once the deserializers are updated of course), right?

@eregon
Copy link
Member

eregon commented Mar 7, 2026

Some stats:
master https://github.com/ruby/prism/actions/runs/22784388008/job/66097760909
this branch https://github.com/ruby/prism/actions/runs/22789229164/job/66112499398?pr=3972

All fields:

master:
Total sizes for top 100 gems:
total source size:      90207647
total serialized size: 113025865
total serialized/total source: 1.253

Stats of ratio serialized/source per file:
average: 1.350
median:  1.343
1st quartile: 0.980
3rd quartile: 1.681
min - max: 0.129 - 4.922

this branch:
Total sizes for top 100 gems:
total source size:      90207647
total serialized size: 124178078
total serialized/total source: 1.377

Stats of ratio serialized/source per file:
average: 1.601
median:  1.609
1st quartile: 1.172
3rd quartile: 1.988
min - max: 0.185 - 5.556

Only semantic fields:

master:
Total sizes for top 100 gems:
total source size:      90207647
total serialized size:  66147370
total serialized/total source: 0.733

Stats of ratio serialized/source per file:
average: 0.800
median:  0.779
1st quartile: 0.568
3rd quartile: 1.007
min - max: 0.076 - 3.675

this branch:
Total sizes for top 100 gems:
total source size:      90207647
total serialized size:  77299583
total serialized/total source: 0.857

Stats of ratio serialized/source per file:
average: 1.051
median:  1.035
1st quartile: 0.760
3rd quartile: 1.301
min - max: 0.080 - 4.286

So total serialized size increases as expected, but the increase is far smaller than total source size so it seems a rather clear gain in retained memory (when doing lazy method loading).

@eregon
Copy link
Member

eregon commented Mar 7, 2026

@eregon @enebo would you both be able to experiment with this branch and tell me what you think? If it works better for JRuby/TruffleRuby I'm happy to just merge this as the default and remove the branching logic inside the deserializers

Conceptually it should be strictly better for retained memory, and it should make it possible to lazily deserialize methods. Right now the laziness for methods in TruffleRuby is by lazily translating the Prism AST of a def to TruffleRuby AST, but the entire Prism AST is still loaded eagerly (because Loader.java currently only supports that, and having to keep the serialized byte[] + source byte[] around for source substrings seems not great, at least currently we don't need to keep the serialized byte[] around).
So instead of keeping the entire Prism AST around for lazy methods we'd keep the serialized byte[] and deserialize on first execution of the method.

In practice if you're thinking of parse performance in TruffleRuby/JRuby then I think we should adapt deserializers as well already before measuring, e.g. to no longer have to handle the "owned string bit".

I checked that the source bytes are only used by org.prism.Loader#loadString and org.prism.Loader.ConstantPool#get, both of which should no longer need the source bytes with this PR.

@eregon
Copy link
Member

eregon commented Mar 7, 2026

Regarding stopping at DefNode in Loader.java to have lazy deserialization, I'd be happy to look into that, though it seems better done separately from this PR.

@enebo
Copy link
Collaborator

enebo commented Mar 7, 2026

I think this looks good. We have plans to do an update on JRuby side this week. Ultimately as @eregon said the big opt for prism will be completely going lazy for methods (leaving serialized sections un-ASTd much less built into IR). Not having to make the AST eagerly for methods we may never call is one step past what we do in legacy parsing.

@eregon
Copy link
Member

eregon commented Mar 7, 2026

I also wanted to say for me this PR with the deserializers changes would be fine to merge, for TruffleRuby POV I think there is no need to benchmark before merging.

@headius
Copy link
Contributor

headius commented Mar 7, 2026

Worth mentioning that lazy def nodes will also help the wasm implementation narrow the gap with native.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Copy & embed any string/constant pool entry in the serialized output

4 participants