616562 - speed up JSString::isStatic()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: patch

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.

Comment 1

[Nicholas Nethercote [inactive]]

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.

Comment 2

[Luke Wagner [:luke]]

(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.

Comment 3

[Nicholas Nethercote [inactive]]

(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.

Comment 4

[Luke Wagner [:luke]]

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 :)

Comment 5

[Luke Wagner [:luke]]

Attachment: optimize static check

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.

Comment 6

[Nicholas Nethercote [inactive]]

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.

Comment 7

[Luke Wagner [:luke]]

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?

Comment 8

[Nicholas Nethercote [inactive]]

(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.

Comment 9

[Luke Wagner [:luke]]

(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.

Comment 10

[Nicholas Nethercote [inactive]]

(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.

Comment 11

[Luke Wagner [:luke]]

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 12

[Nicholas Nethercote [inactive]]

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?

Comment 13

[Luke Wagner [:luke]]

(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 :)

Comment 14

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/cf1850399b0b

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/cf1850399b0b