Skip to content
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.

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

Optimize serialized size for PRISM_SERIALIZE_ONLY_SEMANTICS_FIELDS=1 #2956

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

eregon
Copy link
Member

@eregon eregon commented Jul 24, 2024

From #2924 (comment), average went from 0.799 before #2924 to 0.950 after, with this PR it goes back to 0.800.

As an aside this makes the flags field public again, which is necessary as TruffleRuby uses it like that (the error can be seen at https://github.com/oracle/truffleruby/actions/workflows/prism.yml).

…s set

* $ bundle exec rake serialized_size:topgems
Before:
Total sizes for top 100 gems:
total source size:      90207647
total serialized size:  86284647
total serialized/total source: 0.957

Stats of ratio serialized/source per file:
average: 0.952
median:  0.937
1st quartile: 0.669
3rd quartile: 1.206
min - max: 0.080 - 4.065

After:
Total sizes for top 100 gems:
total source size:      90207647
total serialized size:  69477115
total serialized/total source: 0.770

Stats of ratio serialized/source per file:
average: 0.844
median:  0.825
1st quartile: 0.597
3rd quartile: 1.064
min - max: 0.078 - 3.792
…LDS is set

* Note that we could shift the flags by 2 on serialize & deserialize
  but it does not seems worth it as it does not save serialized size
  in any significant amount, i.e. average was 0.799 before ruby#2924.
* $ bundle exec rake serialized_size:topgems
Before:
Total sizes for top 100 gems:
total source size:      90207647
total serialized size:  69477115
total serialized/total source: 0.770

Stats of ratio serialized/source per file:
average: 0.844
median:  0.825
1st quartile: 0.597
3rd quartile: 1.064
min - max: 0.078 - 3.792

After:
Total sizes for top 100 gems:
total source size:      90207647
total serialized size:  66150209
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
@eregon eregon requested a review from kddnewton July 24, 2024 20:25
@eregon eregon mentioned this pull request Jul 24, 2024
Copy link
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

I'm okay with this, but I was under the impression you wanted the flags for the static literals maybe? Do you want to just exclude node id?

@eregon
Copy link
Member Author

eregon commented Jul 24, 2024

I was under the impression you wanted the flags for the static literals maybe?

I thought initially that would be nice mostly for the Ruby API (#1531) as a module/interface which guarantees a value method (which some of these nodes already have).

I suppose it could be useful for Java compilation to know if a node is a static literal for which the value can be potentially cached in the IR, but in practice that still depends on implementation and representation details so I think the value is fairly limited and at least currently not worth the serialized size overhead.

@eregon
Copy link
Member Author

eregon commented Jul 24, 2024

I did a quick search and for instance the only place we do instanceof Nodes.IntegerNode in TruffleRuby is for https://github.com/oracle/truffleruby/blob/3cd422433deebe3fa664f8c4540811c42ca02e93/src/main/java/org/truffleruby/parser/YARPTranslator.java#L3769-L3785 which doesn't map so nicely to the static literal flag (it covers more than that like ivar reads).

@eregon
Copy link
Member Author

eregon commented Jul 24, 2024

And for completeness, regarding the newline flag, I looked a bit into it today (previous comment about it in #2924 (comment)) but there seems to be no value to it currently in the Java API since it wouldn't really save any work from MarkNewlinesVisitor. (It could be helpful when using something else than MarkNewlinesVisitor to assign the real newline flags though I suppose, but Loader currently always uses MarkNewlinesVisitor).

@eregon eregon merged commit e012072 into ruby:main Jul 25, 2024
56 checks passed
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.

2 participants