Closed Bug 948638 Opened 9 years ago Closed 9 years ago

Always use jsid struct types

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: sstangl, Assigned: sstangl)

References

(Depends on 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file)

Bug 939505 introduced JS_DEBUG for exported headers in order to allow linking against ptrdiff_t jsid mozjs when DEBUG is defined for the parent project. Bug 948099 extends JS_DEBUG to the rest of the engine.

Waldo pointed out that we wouldn't need JS_DEBUG for jsid if we always used jsid struct types, and that we haven't measured the performance impact of making that switch.

The attached patch makes that change. Landing this patch will hopefully let us revert JS_DEBUG.

Octane performance is largely unchanged, except that octane-regexp receives a consistent performance improvement from ~1800 to ~1950 on both x86 and x86_64. I'm not sure why that is, but I'll take it.
Attachment #8345502 - Flags: review?(jwalden+bmo)
Comment on attachment 8345502 [details] [diff] [review]
0001-Use-jsid-struct-types-in-opt-builds.patch

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

::: js/public/Id.h
@@ +33,5 @@
>      size_t asBits;
>      bool operator==(jsid rhs) const { return asBits == rhs.asBits; }
>      bool operator!=(jsid rhs) const { return asBits != rhs.asBits; }
>  };
> +#define JSID_BITS(id) (id.asBits)

Would be nice to get rid of this too, but that can be done in a separate patch, no need to hold this up for that.
Attachment #8345502 - Flags: review?(jwalden+bmo) → review+
Make sure to double check that this patch doesn't increase the number of static constructors; you might have to sprinkle some MOZ_CONSTEXPR about.
Per a Try run at https://tbpl.mozilla.org/?tree=Try&rev=23834f5dc1d0, num_ctors increased from 94 to 123. Looking at the Tryserver history at http://graphs.mozilla.org/graph.html#tests=[[81,23,6]]&sel=none&displayrange=7&datatype=running shows this 123 range occurring intermittently.
Pushed. Will address possible static constructor count increase after some runs.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9acb6b5ed0f
Regression: Mozilla-Inbound - Number of Constructors - CentOS (x86_64) release 5 (Final) - 35.1% increase
---------------------------------------------------------------------------------------------------------
    Previous: avg 94.000 stddev 0.000 of 12 runs up to revision 28587965e647
    New     : avg 127.000 stddev 0.000 of 12 runs since revision a9acb6b5ed0f
    Change  : +33.000 (35.1% / z=0.000)
    Graph   : http://mzl.la/1f5TRZu

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=28587965e647&tochange=a9acb6b5ed0f

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/a9acb6b5ed0f
    : Sean Stangl <sstangl@mozilla.com> - Bug 948638 - Always use jsid struct types. r=Waldo
    : http://bugzilla.mozilla.org/show_bug.cgi?id=948638

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=948638 - Always use jsid struct types
https://hg.mozilla.org/mozilla-central/rev/a9acb6b5ed0f
Assignee: nobody → sstangl
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 949183
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.