Closed
Bug 1181558
Opened 9 years ago
Closed 9 years ago
Reduce stack space of the InlineFrameIterator.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: nbp, Assigned: nbp)
References
Details
Attachments
(2 files, 1 obsolete file)
2.51 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
13.36 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Currently the InlineFrameIterator takes too much stack space, mostly because of the MachineState (600 bytes each) and we have one per snapshot.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8630999 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•9 years ago
|
||
This patch move the storage of the machine state from the SnapshotIterator to the JitFrameIterator, and the computation to the JitFrameIterator::operator++(). I am not completely sure this would be the most efficient, as the JitFrameIterator is part of the Data of the FrameIter, as opposed to the InlineFrameIterator.
Attachment #8631002 -
Flags: review?(jdemooij)
Comment 3•9 years ago
|
||
Comment on attachment 8630999 [details] [diff] [review] part 0 - Remove unused SnapshotIterator constructor. Review of attachment 8630999 [details] [diff] [review]: ----------------------------------------------------------------- Nice
Attachment #8630999 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 4•9 years ago
|
||
This patch move the MachineState to the InlineFrameIterator, if there is one, or to the stack if there is none (such as in case of bailouts, or recovery of instructions).
Assignee: nobody → nicolas.b.pierron
Attachment #8631002 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8631002 -
Flags: review?(jdemooij)
Attachment #8631552 -
Flags: review?(jdemooij)
Comment 5•9 years ago
|
||
Comment on attachment 8631552 [details] [diff] [review] part 1 - Share the machine state between all SnapshotIterators. Review of attachment 8631552 [details] [diff] [review]: ----------------------------------------------------------------- Great. ::: js/src/jit/Registers.h @@ +111,5 @@ > return offsetof(RegisterDump, fpregs_) + reg.getRegisterDumpOffsetInBytes(); > } > }; > > +// Information needed to recover machine register state. This record the Nit: records @@ +125,5 @@ > MachineState() { > + clear(); > + } > + > + void clear() { clear() is only called by the constructor now right? Could we revert this change?
Attachment #8631552 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 6•9 years ago
|
||
Checking with js/xpconnect/tests/chrome/test_bug732665_meta.js on (x64, clang 3.3), before: Max stack size: 1036848 bytes Maximum number of frames: 164 Average frame size: 6323 bytes after: Max stack size: 1036144 bytes Maximum number of frames: 181 Average frame size: 5725 bytes Which corresponds to the 600k of one MachineState, as jandem was reporting to me over IRC. One of the remaining question, is why do we have reserve space for a MachineState on the stack.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5aa2faca75e https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff23939ce8f
https://hg.mozilla.org/mozilla-central/rev/a5aa2faca75e https://hg.mozilla.org/mozilla-central/rev/9ff23939ce8f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•