Closed Bug 1249235 Opened 4 years ago Closed 4 years ago

Store RegExp flags into single slot.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(7 files, 1 obsolete file)

Currently RegExp flags are stored into multiple slots, but they are stored with BooleanValue, and no other types would be used, as they're readonly (am I correct?).  so we could use single slot with Int32Value, with raw RegExpFlags value.  it reduces the RegExpObject's size and possibly improves performance.
so, there was no need to use separated slots after bug 1120169.

AllocKind of RegExpObject is reduced from OBJECT8 to OBJECT4.

octane/regexp score improves about 2%:
  before: 4089
  after:  4176
Assignee: nobody → arai.unmht
Attachment #8720770 - Flags: review?(hv1989)
Changing RegExp.prototype.flags to call all getters before other operations reduces the loadfixedslot instruction.

without this patch, it needs 2 loadfixedslot, before global and unicode.
Attachment #8720776 - Flags: review?(hv1989)
Comment on attachment 8720770 [details] [diff] [review]
Part 1: Store RegExp flags into single slot.

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

Nice!
Attachment #8720770 - Flags: review?(hv1989) → review+
Comment on attachment 8720776 [details] [diff] [review]
Part 2: Optimize RegExp.prototype.flags to load flags at once.

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

I find this odd. In my knowledge the compiler should be able to transform this into one Load.
Looks good. Needinfo'ing me to investigate that.
Attachment #8720776 - Flags: review?(hv1989) → review+
Flags: needinfo?(hv1989)
To be clear, I used m-i 410ef34da2e7 revision and applied Part 1, and executed following code with `IONFLAGS=logs js --ion-offthread-compile=off test.js` with 64bit build on osx.

configure flags: --enable-debug --disable-optimize --enable-warnings-as-errors --enable-nspr-build

code:

let n = 0;
let re = /a/;
function f(R) {
    return R.flags;
}
for (let i = 0; i < 10000; i ++) {
    n += f(re).length;
}


I get func00, func01, and func02.  func00 and func01 are graph for RegExp.prototype.flags, and func02 is the graph that includes for loop and RegExp.prototype.flags.

in func00 and func01, loadfixedslot is executed twice, and in func02 it's executed only once, per single RegExp.prototype.flags call.

maybe func00 and func01 are the graph for not yet fully optimizated cases?
oh, I was wrong, in func02 loadfixedslot is executed twice *before* the loop.
maybe, I've read the graph in totally wrong way.

even with Part 2, there are also 2 loadfixedslot's in func00, func01, and func02, with the code in comment #5.
so what the Part 2 does wasn't what I intended, sorry.
Comment on attachment 8720776 [details] [diff] [review]
Part 2: Optimize RegExp.prototype.flags to load flags at once.

as discussed in IRC, marking Part 2 obsolete, as it does not reduce the loadfixedslot.
will land Part 1 only after some more investigation.
Attachment #8720776 - Attachment is obsolete: true
Depends on: 1249588
Apparently this patch and the others you landed with it, were a win on dromaeo-sunspider-string-validate-input (13% improvement) and made dromaeo-v8-richards stable.
https://hg.mozilla.org/mozilla-central/rev/d1a4b82b556a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
My needinfo is not needed anymore. Arai++ already fixed it.
Flags: needinfo?(hv1989)
You need to log in before you can comment on or make changes to this bug.