Closed Bug 601066 Opened 14 years ago Closed 14 years ago

JM: Eliminate ImmutableSync on x64

Categories

(Core :: JavaScript Engine, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sstangl, Unassigned)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

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.
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.
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
Applies on top of 598839.
Attachment #480373 - Flags: review?(dvander)
Attachment #480373 - Flags: review?(dvander)
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+
http://hg.mozilla.org/tracemonkey/rev/ad0c80eacba7
Whiteboard: fixed-in-tracemonkey
Was backed out: http://hg.mozilla.org/tracemonkey/rev/7b1071bf66fb
Whiteboard: fixed-in-tracemonkey
And you shake it all about,

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