922677 - short-circuit no-op changes to classList

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

While profiling b2g bug 914913 I believe I saw improvements from avoiding no-op changes to the classList property.  I documented this in bug 921808.

While an app can do things like:

  if (!el.classList.contains('hide')) {
    el.classList.add('hide');
  }

It would be nice if this was not necessary.  It seems we can fairly easily short-circuit the logic in nsDOMTokenList.cpp Add() and Remove() to minimize work performed in the no-op case.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: checked-classlist

This patch performs the short-circuit logic and avoids triggering the work in SetAttr() when no changes have been made.

I tried to get a clean profile during the contacts app launch showing the difference, but there is so much going on I was unable to get anything that consistently showed the difference.

As an alternative I did a simple jsperf run with and without the patch at:

  http://jsperf.com/no-op-classlist

This silly micro-bench shows an 80% improvement in the time for a simple no-op classList.add().

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Initial try with attachment 812677 [details] [diff] [review]:

  https://tbpl.mozilla.org/?tree=Try&rev=56fd39bdcfb9

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

That small, opt try looked good.  Here's a wider debug try:

https://tbpl.mozilla.org/?tree=Try&rev=559dac5d443f

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ben, see bug 814014 comment 41....  the spec requires our current behavior, basically.  At least when there are mutation observers present.  So the patch in this bug is no good, unfortunately, unless the spec changes (and we need to add to our test coverage, clearly).

A more interesting question is why a no-op SetAttr call (which is what I assume we have here) is triggering painting, which is what bug 921808 comment 0 seems to say.  Do we not end up taking the OnlyNotifySameValueSet/AttributeSetToCurrentValue fast path?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Ben, see bug 814014 comment 41....  the spec requires our current behavior,
> basically.  At least when there are mutation observers present.  So the
> patch in this bug is no good, unfortunately, unless the spec changes (and we
> need to add to our test coverage, clearly).

Thank you for the info!

> A more interesting question is why a no-op SetAttr call (which is what I
> assume we have here) is triggering painting, which is what bug 921808
> comment 0 seems to say.  Do we not end up taking the
> OnlyNotifySameValueSet/AttributeSetToCurrentValue fast path?

After adding more debugging to those code paths I think I can conclusively say that bug 921808 comment 0 is wrong about it triggering a paint.  In that case there are is one classList.add() and two classList.remove()'s.  The add() is taking the OnlyNotifySameValueSet fast path.  The two removes are taking the fast exit here:

  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMTokenList.cpp#259

Based on the additional profiling I did in comment 0 in this bug and my difficulties getting anything clean, I think the painting improvement I saw was a false positive.

So based on the spec information in comment 4 and the fact that I think the painting was a false positive I'm just going to mark this as WONTFIX.  Sorry for the noise!

Comment 6

[Boris Zbarsky [:bzbarsky]]

No problem.  Please do file a bug if there are cases when no-op setattr is triggering painting or layout; we _definitely_ want to avoid that!

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Yea, I think I just verified that the paint I was seeing was due to an assignment to el.textContent.  I used paint flashing to verify this.  Its a similar case where we are assigning the same value already present in the DOM.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah.  That one is much harder to optimize because the spec requires us to create new DOM nodes...  :(  See bug 725221.