irregexp::InterpretCode: don't heap-allocate |registers|

RESOLVED FIXED in Firefox 57

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: jseward, Unassigned)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

This function evaluates regexp bytecode sequences.  It uses a small array of
int32_ts, |registers|, that are mentioned by the bytecode.  The current
setup for |registers| guarantees that it will always be heap allocated,
unless |numRegisters| is zero, which appears to not happen, according to
some testing.  The reason for the heap allocation is

    Vector<int32_t, 0, SystemAllocPolicy> registers;
    if (!registers.growByUninitialized(numRegisters)) {
       ...

The Vector declaration used creates a vector with an in-line (in this case,
on-stack) capacity of zero.  Since we immediately resize it to a non-zero
capacity, Vector has no option but to reallocate it on the heap.  From
testing Speedometer, |numRegisters| never exceeds 8, and so we could quite
easily accomodate that on the stack, without loss of generality, merely by
initially sizing the vector accordingly.
Here's a typical DHAT record.  See how growByUninitialized is forcing the
allocation to happen.  Also, that the allocation is both ludicrously small
(8.01 bytes) and short lived (961 instructions).

-------------------- 119 of 1000 --------------------
max-live:    32 in 1 blocks
tot-alloc:   21,712 in 2,709 blocks (avg size 8.01)
deaths:      2,709, at avg age 961 (0.00% of prog lifetime)
acc-ratios:  0.12 rd, 1.25 wr  (2,776 b-read, 27,252 b-written)
   at 0x4C2BF9D: malloc (/home/sewardj/VgTRUNK/progress/coregrind/m_replacemalloc/vg_replace_malloc.c:299)
   by 0x1433C67C: js_malloc (gcc-O2-nondebug-jemalloc/dist/include/js/Utility.h:230)
   by 0x1433C67C: js_pod_malloc<int> (gcc-O2-nondebug-jemalloc/dist/include/js/Utility.h:421)
   by 0x1433C67C: maybe_pod_malloc<int> (js/src/jsalloc.h:33)
   by 0x1433C67C: pod_malloc<int> (js/src/jsalloc.h:38)
   by 0x1433C67C: convertToHeapStorage (gcc-O2-nondebug-jemalloc/dist/include/mozilla/Vector.h:936)
   by 0x1433C67C: mozilla::Vector<int, 0ul, js::SystemAllocPolicy>::growStorageBy(unsigned long) (gcc-O2-nondebug-jemalloc/dist/include/mozilla/Vector.h:1027)
   by 0x146E6E22: growByUninitialized (gcc-O2-nondebug-jemalloc/dist/include/mozilla/Vector.h:1153)
   by 0x146E6E22: js::RegExpRunStatus js::irregexp::InterpretCode<unsigned char>(JSContext*, unsigned char const*, unsigned char const*, unsigned long, unsigned long, js::MatchPairs*, unsigned long*) (js/src/irregexp/RegExpInterpreter.cpp:137)
   by 0x1454FDD4: js::RegExpShared::execute(JSContext*, JS::MutableHandle<js::RegExpShared*>, JS::Handle<JSLinearString*>, unsigned long, js::MatchPairs*, unsigned long*) (js/src/vm/RegExpObject.cpp:1170)
   by 0x14731E98: ExecuteRegExpImpl(JSContext*, js::RegExpStatics*, JS::MutableHandle<js::RegExpShared*>, JS::Handle<JSLinearString*>, unsigned long, js::MatchPairs*, unsigned long*) (js/src/builtin/RegExp.cpp:129)
   by 0x14732246: ExecuteRegExp(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSString*>, int, js::MatchPairs*, unsigned long*, js::RegExpStaticsUpdate) (js/src/builtin/RegExp.cpp:953)
   by 0x14733481: js::RegExpTesterRaw(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSString*>, int, int*) (js/src/builtin/RegExp.cpp:1173)
Created attachment 8893778 [details] [diff] [review]
The obvious fix
(Reporter)

Updated

3 months ago
Attachment #8893778 - Flags: review?(bhackett1024)
Attachment #8893778 - Flags: review?(bhackett1024) → review+

Comment 3

2 months ago
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/085a687a81d9
irregexp::InterpretCode: don't heap-allocate |registers|.  r=bhackett.

Comment 4

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/085a687a81d9
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.