Closed Bug 997590 Opened 10 years ago Closed 10 years ago

Create RegExpStaticsObject lazily

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [MemShrink:P3])

Attachments

(3 files, 1 obsolete file)

Every global object gets a RegExpStaticsObject, whether it needs it or not. On
64-bit platforms, this is about 240 bytes worth of stuff (the object plus the
RegExpStatics struct hanging off it).

Also, I'm investigating compartment overhead in general and this is one cause,
so getting it out of the way will be nice.
The else-branch here is impossible -- globals currently always have a
RegExpStaticsObject.
Attachment #8407992 - Flags: review?(sstangl)
At start-up, this reduces the number of RegExpStaticsObjects from 218 (one per
compartment) to 68, saving about 36 KB.

Most of the changes are to account for getRegExpStatics() now being fallible.

A couple of JSAPI functions had their return type changed from void to bool as
a result.
Attachment #8407996 - Flags: review?(sstangl)
Attachment #8407992 - Flags: review?(sstangl) → review+
Comment on attachment 8407996 [details] [diff] [review]
(part 2) - Create RegExpStaticsObjects lazily

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

::: js/src/jit/IonBuilder.cpp
@@ +9381,5 @@
>      types::TypeObjectKey *typeObj = types::TypeObjectKey::get(&script()->global());
>      if (!typeObj->hasFlags(constraints(), types::OBJECT_FLAG_REGEXP_FLAGS_SET)) {
> +        RegExpStatics *res = script()->global().getRegExpStatics(analysisContext);
> +        if (!res)
> +            return false;

This must be "return oom();" -- returning false without providing a reason will cause Ion to mark the function as uncompilable.
Attachment #8407996 - Flags: review?(sstangl) → review+
Whiteboard: [MemShrink] → [MemShrink:P3]
This function has a hazard -- create() can trigger GC, which means that |this|
needs to be rooted. I tried using a |self| pointer, because converting to a
static function would be a pain, as this function has many callers. But I'm
getting various jit-test failures -- crashes and the like.

sfink, any idea what I'm doing wrong?
Attachment #8412192 - Flags: review?(sphink)
Comment on attachment 8412192 [details] [diff] [review]
(part 2b) - Fix a hazard (BUSTED)

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

Ah... turns out the |cx| being passed in from IonBuilder::jsop_regexp() is 0x0, and that's causing the crashes. Now I just have to work out why it's 0x0 and how to get a non-0x0 |cx| in that function.
Attachment #8412192 - Flags: review?(sphink)
This patch fixes a rooting hazard (for |this|) in getRegExpStatics(). It turned
out a little tricky -- sometimes the |cx| passed in from jsop_regexp() was
null, which caused the rooting code to crash. Fortunately the code in
jsop_regexp() is debug-only, so I just made the assertion conditional on having
already instantiated the statics.

I'll fold this into part 2 before landing.
Attachment #8413475 - Flags: review?(sstangl)
Attachment #8412192 - Attachment is obsolete: true
Attachment #8413475 - Flags: review?(sstangl) → review+
https://hg.mozilla.org/mozilla-central/rev/2e077142ca73
https://hg.mozilla.org/mozilla-central/rev/91347e932220
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1019825
You need to log in before you can comment on or make changes to this bug.