Inline JSLinearString::mark

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

unspecified
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
JSLinearString::mark is defined in vm/String.cpp and is not available for inlining in jsgc.cpp resulting in useless calls when marking strings.
(Assignee)

Comment 1

6 years ago
Created attachment 578547 [details] [diff] [review]
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%.
(Assignee)

Comment 2

6 years ago
Created attachment 578605 [details] [diff] [review]
v2

The new version fixes broken ScanRope in v1 and adds comments. Now it passes the tests.
Attachment #578547 - Attachment is obsolete: true
Attachment #578605 - Flags: review?(wmccloskey)
(Assignee)

Comment 3

6 years ago
Created attachment 578607 [details] [diff] [review]
v3

The argument of ScanLinearString should be JSLinearString, not JSString.
Attachment #578605 - Attachment is obsolete: true
Attachment #578605 - Flags: review?(wmccloskey)
Attachment #578607 - Flags: review?(wmccloskey)
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?
Attachment #578607 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 5

6 years ago
(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.
(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.
(Assignee)

Comment 7

6 years ago
(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

6 years ago
(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 :)
Yeah, I just missed it.  Please do remove isRope and asRope.
(Assignee)

Comment 10

6 years ago
(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.
(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

6 years ago
(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.
(Assignee)

Comment 13

6 years ago
Created attachment 578747 [details] [diff] [review]
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.
Attachment #578607 - Attachment is obsolete: true
Attachment #578747 - Flags: review+
(Assignee)

Comment 14

6 years ago
(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());
(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.
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.
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.
(Assignee)

Comment 18

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/014f175b66d0
https://hg.mozilla.org/mozilla-central/rev/014f175b66d0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Updated

5 years ago
Depends on: 726934
You need to log in before you can comment on or make changes to this bug.