Closed Bug 707114 Opened 8 years ago Closed 8 years ago

Inline JSLinearString::mark

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(1 file, 3 obsolete files)

JSLinearString::mark is defined in vm/String.cpp and is not available for inlining in jsgc.cpp resulting in useless calls when marking strings.
Attached patch v1 (obsolete) — Splinter Review
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%.
Attached patch v2 (obsolete) — Splinter Review
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)
Attached patch v3 (obsolete) — Splinter Review
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+
(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.
(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?
(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.
(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.
(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.
Attached patch v4Splinter Review
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/014f175b66d0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Depends on: 726934
You need to log in before you can comment on or make changes to this bug.