707114 - Inline JSLinearString::mark

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

JSLinearString::mark is defined in vm/String.cpp and is not available for inlining in jsgc.cpp resulting in useless calls when marking strings.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch replaces JSLinearString:mark with ScanLinearString, an inline function in jsgcmark.cpp. The later assumes that the string should be already marked (like other Scan* functions do).

The patch also optimizes ScanRope so it uses the mark stack only when both its children are ropes.

The following test, when run with MOZ_GCTIMER set to stdout, shows the effect of the patch:

function test() {
    var a = [];
    var N = 1e7;
    for (var i = 0; i != N; ++i) {
	a.push(String.fromCharCode(0, 0, 0, 0));
    }
    print("Caling the GC...");
    gc();
    print("Done.");
}

test();

Without the patch the output is:

13.040000 12.567000 0.055000
26.427000 26.369000 0.039000
40.673000 40.615000 0.039000
55.287000 55.228000 0.039000
66.971000 66.901000 0.049000
Caling the GC...
76.692000 76.635000 0.037000
Done.
424.406000 0.172000 424.223000


With the patch that becomes:

9.959000 9.512000 0.064000
19.861000 19.803000 0.040000
30.405000 30.343000 0.041000
41.654000 41.595000 0.038000
50.221000 50.150000 0.049000
Caling the GC...
55.397000 55.340000 0.037000
Done.
427.136000 0.196000 426.929000

That is, the string mark time is improved by more than 25%.

Comment 2

[Igor Bukanov]

Attachment: v2

The new version fixes broken ScanRope in v1 and adds comments. Now it passes the tests.

Comment 3

[Igor Bukanov]

Attachment: v3

The argument of ScanLinearString should be JSLinearString, not JSString.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 578607 [details] [diff] [review]
v3

Review of attachment 578607 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: js/src/jsgcmark.cpp
@@ +719,5 @@
>  
>  static inline void
> +ScanLinearString(GCMarker *gcmarker, JSLinearString *str)
> +{
> +    JS_ASSERT(str->isMarked());

Could you put a JS_COMPARTMENT_ASSERT here? It's redundant, but one day we might call ScanLinearString from somewhere else.

@@ +774,5 @@
> +    if (str->markIfUnmarked()) {
> +        if (str->isLinear()) {
> +            ScanLinearString(gcmarker, &str->asLinear());
> +        } else {
> +            JS_ASSERT(str->isRope());

Could you move this assertion to ScanRope?

Comment 5

[Igor Bukanov]

(In reply to Bill McCloskey (:billm) from comment #4)
> @@ +774,5 @@
> > +    if (str->markIfUnmarked()) {
> > +        if (str->isLinear()) {
> > +            ScanLinearString(gcmarker, &str->asLinear());
> > +        } else {
> > +            JS_ASSERT(str->isRope());
> 
> Could you move this assertion to ScanRope?

ScanRope takes JSRope so asserting there is too late. I will just remove it - str->asRope() on the following line will check it in any case.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Igor Bukanov from comment #5)
> ScanRope takes JSRope so asserting there is too late. I will just remove it
> - str->asRope() on the following line will check it in any case.

I had a problem the other day where I had a JSRope*, and then it got flattened, so it was no longer a JSRope. Having the assertion just seems safer to me.

Comment 7

[Igor Bukanov]

(In reply to Bill McCloskey (:billm) from comment #6)
> I had a problem the other day where I had a JSRope*, and then it got
> flattened, so it was no longer a JSRope. Having the assertion just seems
> safer to me.

I suppose the assert then should go to the part of the code that flattened the string. Also it would be strange to have such assert in ScanRope while not having it in ScanLinerString. And adding the assert to the latter in a straightforward way cannot be done as JSLinearString explicitly remove asLiner(), isLinear() methods using MOZ_DELETE;

Luke: do you know on those isLinear() MOZ_DELETE in JSLinearString and why isRope() was not removed from JSRope in a similar way?

Comment 8

[Luke Wagner [:luke]]

(In reply to Igor Bukanov from comment #7)
> Luke: do you know on those isLinear() MOZ_DELETE in JSLinearString and why
> isRope() was not removed from JSRope in a similar way?

Unless I'm missing a subtlety, probably b/c Waldo missed it :)

Comment 9

[Jeff Walden [:Waldo]]

Yeah, I just missed it.  Please do remove isRope and asRope.

Comment 10

[Igor Bukanov]

(In reply to Jeff Walden (remove +bmo to email) from comment #9)
> Yeah, I just missed it.  Please do remove isRope and asRope.

To clarify: the idea is to trust the static type of the JSString and do not use asserts to confirm that the dynamic type still matches the static one. Rather the asserts should only be at the points when we do the casts. I suppose this works for strings since the dynamic type of the string should not change after its initialization.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Igor Bukanov from comment #10)
> To clarify: the idea is to trust the static type of the JSString and do not
> use asserts to confirm that the dynamic type still matches the static one.
> Rather the asserts should only be at the points when we do the casts. I
> suppose this works for strings since the dynamic type of the string should
> not change after its initialization.

But that's exactly the problem--the dynamic type *can* change after initialization. Consider the following:
  JSRope *rope = ...;
  JS_ASSERT(rope->isRope()); // passes
  rope->flatten(cx);
  JS_ASSERT(rope->isRope()); // fails

That's why I think it's nice to have the assert. Luke suggested that, even after isRope is removed from rope, we can get the same effect as follows:
  JS_ASSERT(static_cast<JSString *>(rope)->isRope());
He thinks this is non-horrible.

Comment 12

[Luke Wagner [:luke]]

(In reply to Bill McCloskey (:billm) from comment #11)
Waldo had an idea that is even more non-horrible, even good: for every method on string class X, put a JS_ASSERT(isX()) at the head of every X method.  We already do this in a few places.

Comment 13

[Igor Bukanov]

Attachment: v4

In the new version I have added the asserts that confirm the static type of the string. As the GC navigates the object graph it may find things that the rest of the code may never touch again so the extra asserts have a sufficient utility to tolerate the ugliness of those type confirmations.

I have not removed JSRope::asRope as I will file another bug about it.

Comment 14

[Igor Bukanov]

(In reply to Bill McCloskey (:billm) from comment #11)
> But that's exactly the problem--the dynamic type *can* change after
> initialization. Consider the following:
>   JSRope *rope = ...;
>   JS_ASSERT(rope->isRope()); // passes
>   rope->flatten(cx);
>   JS_ASSERT(rope->isRope()); // fails
> 
> That's why I think it's nice to have the assert.

Perhaps instead of the C++ types we should implement a static analysis that supports those type mutations... 

> Luke suggested that, even
> after isRope is removed from rope,

I think we should remove just asRope, keep isRope in JSRope and re-enable isLinear() in JSLinearString?

>   JS_ASSERT(static_cast<JSString *>(rope)->isRope());

Here is a less ugly version:

   JS_ASSERT(rope->JSString::isRope());

Comment 15

[Jeff Walden [:Waldo]]

(In reply to Igor Bukanov from comment #14)
> I think we should remove just asRope, keep isRope in JSRope and re-enable
> isLinear() in JSLinearString?

Having isLinear() in JSLinearString makes it possible to write bizarre truisms in code:

  T method(JSLinearString* str)
  {
    ...
    if (str->isLinear())
    {
      ...always hit
    }
    else
    {
      ...dead code...
    }
    ...
  }

This is not a theoretical issue: I deleted these vacuous methods because I encountered an instance of them being used that way -- linearStr->ensureLinear(cx), in particular.  I don't want it to be possible to make that mistake.  isLinear and all the other methods should remain deleted.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

What if isLinear and isRope are present on their respective classes in debug builds? That we we can only use them in asserts? The code in comment 15 would fail to compile in opt builds.

Comment 17

[Jeff Walden [:Waldo]]

That's even worse!  I can't remember the last time I did an opt build.  :-)

If a check really needs to be inserted here, and simply relying on checks included at the start of all JSString subclass member functions is inadequate, I think Igor's less ugly version of JS_ASSERT(rope->JSString::isRope()); is the way to go.

Comment 18

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/014f175b66d0

Comment 19

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/014f175b66d0