JM: Eliminate ImmutableSync on x64

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: sstangl, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
With the patch in bug 598839, x64 gets better syncing in the non-syncFancy() case. However, ImmutableSync is untouched, since it is essentially a minor reimplementation of FrameState semantics that would either have to be heavily modified or eliminated.

Elimination is probably the better option -- we can get rid of ~250LOC and an entire class and all associated buffers and memory by being slightly more clever with register use.
(Reporter)

Updated

7 years ago
Depends on: 598839
Need a plan of attack and justification here. The original syncing code was "clever" - to get good register movement it had to be too clever, and it was unmaintainable. We were constantly tracking down edge cases where registers weren't synced or restored properly.

With IM, we know exactly what problems need to be solved and they are nicely encapsulated there.

If the problems (which weren't described in this bug) are x64 specific, then an x64-specific ImmutableSync could be the answer.
(Reporter)

Comment 2

7 years ago
Changed bug title to 'Eliminate ImmutableSync on x64'. Since x64 can trivially do whole-Value loads and stores, and we have a ValueReg always available, it is very simple to never drop out of FrameState::sync().

The numbers below are from this code. ImmutableSync is not defined for 64-bit builds, since it is not necessary. A small amount of memory is saved.

SunSpider is not affected: it's difficult to distinguish from noise. Raytrace and Richards are the only tests on which any change is noticeable.

=============================================================================

** TOTAL **:      1.011x as fast    3814.1ms +/- 0.8%   3774.3ms +/- 0.7%     

=============================================================================

  v8:             1.011x as fast    3814.1ms +/- 0.8%   3774.3ms +/- 0.7%
    crypto:       -                  473.9ms +/- 1.2%    469.0ms +/- 1.1%
    deltablue:    -                 1083.8ms +/- 2.1%   1078.9ms +/- 2.0%
    earley-boyer: *1.015x as slow*   415.5ms +/- 0.7%    421.6ms +/- 0.6%
    raytrace:     1.045x as fast     406.6ms +/- 1.5%    389.1ms +/- 2.3%
    regexp:       -                  489.6ms +/- 0.7%    489.4ms +/- 0.5%
    richards:     1.031x as fast     642.3ms +/- 0.1%    622.8ms +/- 0.2%
    splay:        ??                 302.4ms +/- 1.8%    303.5ms +/- 1.8%
Summary: JM: Eliminate ImmutableSync → JM: Eliminate ImmutableSync on x64
(Reporter)

Comment 3

7 years ago
Created attachment 480373 [details] [diff] [review]
Eliminate syncFancy() and ImmutableSync on x64.

Applies on top of 598839.
Attachment #480373 - Flags: review?(dvander)
(Reporter)

Updated

7 years ago
Attachment #480373 - Flags: review?(dvander)
(Reporter)

Comment 4

7 years ago
Created attachment 481427 [details] [diff] [review]
Eliminate syncFancy() and ImmutableSync on x64, Second Take

Applies on top of last patch from bug 598839.

If you would like to hijack this bug to separate FrameState between the different architectures, that would be fine -- otherwise, the split can be done in the next bug.
Attachment #480373 - Attachment is obsolete: true
Attachment #481427 - Flags: review?(dvander)
Attachment #481427 - Flags: review?(dvander) → review+
(Reporter)

Comment 5

7 years ago
http://hg.mozilla.org/tracemonkey/rev/ad0c80eacba7
Whiteboard: fixed-in-tracemonkey
(Reporter)

Comment 6

7 years ago
Was backed out: http://hg.mozilla.org/tracemonkey/rev/7b1071bf66fb
Whiteboard: fixed-in-tracemonkey
(Reporter)

Comment 7

7 years ago
And you shake it all about,

http://hg.mozilla.org/tracemonkey/rev/d0fd3ff9722e
Whiteboard: fixed-in-tracemonkey

Comment 8

7 years ago
http://hg.mozilla.org/mozilla-central/rev/d0fd3ff9722e
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.