517112 - WriteBarriers should restore inline identity-check short circuits

Status: VERIFIED WONTFIX

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Steven Johnson]

Attachment: Patch

MMGC WriteBarrier classes used to short-circuit if the new value matched the old one, but no longer do. Profiling in Flash apps shows that WriteBarrier::set() is called with tNew==tOld ~50% of the time, and WriteBarrierRC::set() with tNew==tOld ~67% of the time. Patch adds a simple short circuit; this shaves up to 5% of total runtime from certain pathological cases.

Comment 1

[Lars T Hansen]

(In reply to comment #0)
> Created an attachment (id=401139) [details]
> Patch
> 
> MMGC WriteBarrier classes used to short-circuit if the new value matched the
> old one, but no longer do. Profiling in Flash apps shows that
> WriteBarrier::set() is called with tNew==tOld ~50% of the time, and
> WriteBarrierRC::set() with tNew==tOld ~67% of the time. Patch adds a simple
> short circuit; this shaves up to 5% of total runtime from certain pathological
> cases.

I need detail on "profiling in Flash" before I will let this pass.  Are we talking about a broad spectrum of apps or a single case?  And what is the impact of the change across a spectrum of apps?

Comment 2

[Steven Johnson]

(In reply to comment #1)
> I need detail on "profiling in Flash" before I will let this pass.  Are we
> talking about a broad spectrum of apps or a single case? 

Busted! Yeah, we're talking about two specific apps here, both of which are heavy WriteBarrier users. Do you suspect that other apps would see a downgrade in performance? Do you have a specific spectrum of apps in mind?

Comment 3

[Lars T Hansen]

(In reply to comment #2)

> Do you suspect that other apps would see a downgrade
> in performance?

I'm not sure.  It's a cheap check and an expensive barrier so I'm not ready to say that it will be noticed.  I'm just trying to exercise due diligence...

> Do you have a specific spectrum of apps in mind?

Not really, but I'd be more comfortable about seeing data from 10 apps than two.  For example.

Rephrasing a little bit, where are all these self-assignments coming from?  Are they eg DRCWB or DWB destructors that store NULL in fields that already were NULL?  Are they dodgily written apps?  Are they JIT or interpreter bugs?  Is there something I don't understand?  67% of assignments being self-assignment is astonishingly high to me, as is 50% for that matter.

Comment 4

[Steven Johnson]

(In reply to comment #3)
> seeing data from 10 apps

I'll run a better sample, for due diligence, Mac+Win. If I see better-or-at-least-no-worse perf across the board, can I assume a pre-emptive review+?

> Rephrasing a little bit, where are all these self-assignments coming from?  Are
> they eg DRCWB or DWB destructors that store NULL in fields that already were
> NULL?  Are they dodgily written apps?  

Mostly. There's lots of Flash glue code that creates an object with (say) a DWB field, then initializes the field to NULL (redundantly). Likewise the duplicate stores, mostly during object creation phase (but object creation is very frequent in certain classes of flash app). Would be nice to scrub thru all that glue code and smarten it up, but that's a major task.

(The drawback to smart pointers is that an assignment looks as cheap as a normal pointer, which it definitely aint)

Comment 5

[Lars T Hansen]

(In reply to comment #4)
> (In reply to comment #3)
> > seeing data from 10 apps
> 
> I'll run a better sample, for due diligence, Mac+Win. If I see
> better-or-at-least-no-worse perf across the board, can I assume a pre-emptive
> review+?

Sure.

Thanks for the other info, it makes sense.  We should make sure the JIT optimizes known-NULL stores to the maximum extent possible.  If 67% of object stores are truly same-value stores then arguably the JIT should be doing the checking so that there's no call-out to determine that we're not going to be doing anything.

Comment 6

[Steven Johnson]

(In reply to comment #5)
> If 67% of object
> stores are truly same-value stores then arguably the JIT should be doing the
> checking so that there's no call-out to determine that we're not going to be
> doing anything.

JIT doesn't come into play here at all; the numbers I measured were purely for the WriteBarrier[RC] classes, which don't interact with the JIT at all -- they are purely used by C++ code.

Comment 7

[Steven Johnson]

After some profiling, more than meets the eye here -- I think a very few pathological stupidities (in Flash) are causing the bulk of this, so we may be better off *without* the inline check. More info to come.

Comment 8

[Steven Johnson]

After further investigation, I don't think optimization pays for itself; tracking down the worst upstream offenders and fixing those looks to be a better option. Changing bug to WONTFIX.