Implement strings' length property with a plain old data property

RESOLVED FIXED in mozilla5

Status

()

RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
mozilla5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Created attachment 518314 [details] [diff] [review]
Patch, length already pretty well-tested elsewhere so doesn't need any

This is atop the patch for bug 640072 (with obvious changes to DefineConstructorAndPrototype to actually use the specified key, not JSProto_RegExp).  It's a pretty straightforward transliteration of bug 640072's patch to apply to String.

I see scary comments by some of the New* methods that suggest it really is necessary to use different ones to create the prototype object and to create subsequent String objects.  That's a shame if true, because it's easy to create but forget to init, and it'd be easy to not notice the mistake.  I'd love to see one JSObject::createStringObject (maybe for RegExp too, although I might be forgetting a complicating factor there), but I don't think I understand empty shapes well enough now to figure out how to make one, assuming that's possible -- a shame, since there's a handful of random scattered places where we create String objects for strings .
Attachment #518314 - Flags: review?(jorendorff)
(Assignee)

Comment 1

8 years ago
Created attachment 522161 [details] [diff] [review]
Updated for tree churn
Attachment #518314 - Attachment is obsolete: true
Attachment #518314 - Flags: review?(jorendorff)
Attachment #522161 - Flags: review?(jorendorff)
Comment on attachment 522161 [details] [diff] [review]
Updated for tree churn

Good stuff.
Attachment #522161 - Flags: review?(jorendorff) → review+
Blocks: 640724
(Assignee)

Comment 3

8 years ago
http://hg.mozilla.org/tracemonkey/rev/474e167e344a
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla2.2
http://hg.mozilla.org/mozilla-central/rev/474e167e344a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 650621
Why are the initialStringShape and initialRegExpShape stored per-compartment?

There is a reasonably important invariant used by JITs that two objects with the same shape have the same prototype (this is also critical for plans to move obj->prototype into the shape), but there can be multiple String.prototype or RegExp.prototype objects in the same compartment and presumably different string or regexp objects associated with different prototypes could end up having the same shape.
(In reply to comment #5)
> Why are the initialStringShape and initialRegExpShape stored per-compartment?
> 
> There is a reasonably important invariant used by JITs that two objects with
> the same shape have the same prototype (this is also critical for plans to
> move obj->prototype into the shape), but there can be multiple
> String.prototype or RegExp.prototype objects in the same compartment and
> presumably different string or regexp objects associated with different
> prototypes could end up having the same shape.

New bug on file?

Compartment per global may fix this.

/be

Comment 7

7 years ago
If I read comment 5 correctly then it makes sense that the shapes would need to be per-global so CPG would fix this.  Its not close, so sounds like these suckers need to be stashed on the global object.  When CPG does arrive, we can undo all this global-reserved-slots hackery and just stick things on the compartment.
No longer blocks: 640724
You need to log in before you can comment on or make changes to this bug.