Closed Bug 1181558 Opened 9 years ago Closed 9 years ago

Reduce stack space of the InlineFrameIterator.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(2 files, 1 obsolete file)

Currently the InlineFrameIterator takes too much stack space, mostly because of the MachineState (600 bytes each) and we have one per snapshot.
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 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+
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 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+
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: