Revisit symbol hash and marshal fixes from #4576 #5460
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The problem here was that when using ObjectSpace (or object_id, or FFI) we need to insert some hidden state into objects. This state goes into the variable table, but does not get marshaled. However, if we had both reserved space for instance variables and for one of these hidden variables, we always would proceed to marshal the object as though it might have instance variables. If none of the non-hidden variables had been set, we marshaled the object as having instance variables, but of zero length. This broke several marshal specs, since objects that would otherwise have no instance variable were marshaled as such. This fix uses a second check against the variable list to ensure that there are real variables to marshal (or an encoding variable) before proceeding to emit the instance variable sigil and list.
This is not directly used other than to provide a more accurate guess as to whether there are any non-hidden variable in use by a given type. 0b6e3c0 fixes the original issue (marshal dumping zero-length instance var lists). The accessors of the lazy fields are now deprecated. Use the read or write calls to get an accessor for the hidden fields.
In MRI most of these types are derived from the internal DATA class.
We would like to have the table consider encoding, but until we only use ByteList for identifiers this is not possible. We still use Java's String in many places, in which case we only have access to the raw bytes and not the encoding they should be associated with, so the String-based lookup would never be able to add this additional mask. Unfortunately removing this additional hash dimension breaks one Symbol marshal spec, but it was broken before this work as well. This was the cause of Kernel#eval regressions in jruby#4576.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces #4576. I will re-port each fix as necessary.