Closed Bug 616562 Opened 9 years ago Closed 9 years ago

speed up JSString::isStatic()

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
Currently isStatic() involves a lot of range testing.  This patch (applied on top of those in bug 609440) crams static-ness into JSString::lengthAndChars so that it can be tested with a mask and compare.  If I time 100 gc() calls for this live rope:

  var u = "";
  for (var i = 0; i < 100000; ++i)
      u = "asdfasdfasdf" + u + "12341234234";

shows 20% speedup.  There is also speedup for normal strings.  Marking this live array of flat strings:

  var u = "asdfasdasdfasdfasdfasdf";
  var arr = [];
  for (var i = 0; i < 10000; ++i)
      arr.push(u);

shows an 8% speedup.
Summary: speed up isStatic() → speed up JSString::isStatic()
So you are using 5 bits of lengthAndFlags now for type/flag info?  But you haven't changed MAX_LENGTH, and shouldn't FLAT_MODE_MASK be JS_BITMASK(5)?

I also think bug 613457 should land before we start adding more complexity to strings.
(In reply to comment #1)
> So you are using 5 bits of lengthAndFlags now for type/flag info?

no

> I also think bug 613457 should land before we start adding more complexity to
> strings.

Its just a simple representation change; not exactly going to break the complexity bank.  I can wait though.
(In reply to comment #2)
> (In reply to comment #1)
> > So you are using 5 bits of lengthAndFlags now for type/flag info?
> 
> no

Oh, I see, you're using ATOMIZED|EXTENSIBLE to represent STATIC.  Hmm.  If we're going to move from using flags to effectively using enums to represent string state, I think it should be more explicit, maybe like https://bugzilla.mozilla.org/attachment.cgi?id=491799 from bug 613457.  Whatever we do, it needs to be documented clearly.

> > I also think bug 613457 should land before we start adding more complexity to
> > strings.
> 
> Its just a simple representation change; not exactly going to break the
> complexity bank.  I can wait though.

I have two JSString patches that are bit-rotting pretty fast already, I'd prefer to not make the situation worse :)

I like the idea in general, I should add -- both the speed-ups and the fact that static-ness is represented explicitly in JSString.
Yes.  I totally intended to write a comment explaining the new bit-packing scheme, I promise, I just wanted to throw up a patch before leaving for BBQ-friday :)
I wrote the comment for JSString representation on top of this patch, so this patch leaves it as a FIXME.  Since the original filing, NonTypedStringMarker was changed to call isStatic fewer times, but I still see a 10% speedup on the rope-marking micro-bench in comment 0.
Attachment #495074 - Attachment is obsolete: true
Attachment #519300 - Flags: review?(nnethercote)
Blocks: 613457
I'm a bit confused by the interaction between this patch and the one from bug 613457.  Does it make sense to land that patch, then rebase this one?  That'd help me.
This one changes the representation of strings, bug 613457 changes the interface.  Because bug 613457 also provides big beefy comments for the representation, I wanted to do it after the representation change.  It would be most unpleasant to commute the patches.  Perhaps you could read the comments in bug 613457?
(In reply to comment #7)
> Perhaps you could read the comments in bug 613457?

I have read the comments;  I already gave you r+ on that patch.  I was just hoping to make my life easier.  Having to review two large patches that overlap like this kinda sucks.
(In reply to comment #8)
> (In reply to comment #7)
> > Perhaps you could read the comments in bug 613457?
> 
> I have read the comments;  I already gave you r+ on that patch.  I was just
> hoping to make my life easier.  Having to review two large patches that overlap
> like this kinda sucks.

Ironically, I split the representation vs. type/syntactic changes into two patches specifically to make your life easier.  But since you already looked through the other and, it sounds like, looked through the representation changes, it sounds like this one can be auto-r+'d as it is wholly subsumed.
(In reply to comment #9)
> 
> Ironically, I split the representation vs. type/syntactic changes into two
> patches specifically to make your life easier.  

Thanks for the effort.  I must have missed or misunderstood the explanation of the order in which I should have reviewed the patches... and I'm still not clear on that.

> But since you already looked
> through the other and, it sounds like, looked through the representation
> changes, it sounds like this one can be auto-r+'d as it is wholly subsumed.

Can you explain exactly what is the relation between these two patches?  Does one apply on top of the other?  "Subsume" suggests to me that this patch is a strict sub-patch of the other patch, but that doesn't appear to be the case.
Sorry, communication fail; bug 613457 applies on top of this patch.  All it does is change the representation to make JSString::isAtom a bit test vs. 3 inclusion checks.  Bug 613457 leaves the representation unchanged so if you vetted the encoding (viz., of lengthAndFlags) in that bug, then you are done here.
Comment on attachment 519300 [details] [diff] [review]
optimize static check


>-    static inline bool isStatic(void *ptr) {
>+    static inline bool isGCThingStatic(void *ptr) {

Why the name change?  isStatic() seems better to me.  Are there
non-GCThing static strings?
Attachment #519300 - Flags: review?(nnethercote) → review+
(In reply to comment #12)
You're right, and that's what will land with bug 613457 -- I just forgot to hg qpop before renaming it back to isStatic :)
http://hg.mozilla.org/tracemonkey/rev/cf1850399b0b
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/cf1850399b0b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.