Reduce usage of object_id in core gems
#9276
Conversation
amomchilov
force-pushed
the
reduce-object_id-use
branch
3 times, most recently
from
3d686f7
to
9a11a34
Compare
| @@ -1232,10 +1232,7 @@ ensure_class_or_module(VALUE obj) | |||
| static VALUE | |||
| hidden_identity_hash_new(void) | |||
| { | |||
| VALUE hash = rb_ident_hash_new(); | |||
|
|
|||
| RBASIC_CLEAR_CLASS(hash); /* hide from ObjectSpace */ | |||
There was a problem hiding this comment.
Looking through uses of rb_ident_hash_new, I ran into this. Most cases use rb_obj_hide, but this uses RBASIC_CLEAR_CLASS. Is the difference intentional?
There was a problem hiding this comment.
@nobu Do you have any context on this?
There was a problem hiding this comment.
This line predates commit:adf1c94ffe75, where rb_obj_hide was added.
Just added to fix https://bugs.ruby-lang.org/issues/8204, but didn't change already existing code all.
Maybe for unnecessary SPECIAL_CONST_P check.
It would be OK to replace this line with rb_obj_hide, I think.
lib/reline/kill_ring.rb
Outdated
| def ==(other) | ||
| object_id == other.object_id | ||
| equal?(other) | ||
| end |
There was a problem hiding this comment.
Alternatively, I could have deleted the Struct#== implementation with:
| def ==(other) | |
| object_id == other.object_id | |
| equal?(other) | |
| end | |
| remove_method(:==) |
This would cause the Object#== implementation to be inherited (which is identity-based), but it's much less clear, though.
object.c
Outdated
| @@ -98,6 +98,15 @@ rb_obj_embedded_size(uint32_t numiv) | |||
| return offsetof(struct RObject, as.ary) + (sizeof(VALUE) * numiv); | |||
| } | |||
|
|
|||
| /*! | |||
There was a problem hiding this comment.
Unrelated, but this was unclear (what does "hide" mean?) and didn't have any docs, so I just added some.
There was a problem hiding this comment.
Hide the object from ObjectSpace, as written in hidden_identity_hash_new comment.
There was a problem hiding this comment.
Heh yeah I got it, but I didn’t in first read, which is why I added the doc comment for future readers
|
@nobu ah interesting… Would I need to write the patch in each of the two gems in a way that’s backwards-compatible with the other gem being in the older version? Update: I think I figured it out. I opened:
|
amomchilov
force-pushed
the
reduce-object_id-use
branch
2 times, most recently
from
9474caa
to
62fbca8
Compare
It was moved there once, but partially rewritten as an extension library at #2576. |
amomchilov
force-pushed
the
reduce-object_id-use
branch
from
62fbca8
to
5b98bf5
Compare
amomchilov
force-pushed
the
reduce-object_id-use
branch
from
5b98bf5
to
df9cd4d
Compare
|
Hey @nobu, I incorporated the changes you mentioned. Could you please merge what's left of this PR, and review the smaller PRs I opened on the upstream gems? |
Related: ruby/psych#663
Since Ruby 2.7,
object_idbecame more expensive, particularly on its first call for a particular object. @jemmaissroff has a great blog post about it.This PR aims to minimize its use by:.objetc_idvalues withHash#compare_by_identityobject_idcomparisons withequal?In both cases, we can trigger fewer accidental object ID assignments.Update: All of the "meat" of this PR was moved out to the upstream gems' repos. What's left is just a few small changes like adding docs and fixing a typo etc.