Closed Bug 845205 Opened 11 years ago Closed 6 years ago

Do not reset the style context when removing unset properties

Categories

(Core :: CSS Parsing and Computation, defect, P4)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1435139

People

(Reporter: AdamBugzilla, Assigned: bkelly)

References

(
URL
)

Details

(Keywords: perf, Whiteboard: [u= s= c= p=5 ])

Attachments

(1 file, 7 obsolete files)

Updated patch.
8.24 KB, patch
bkelly
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20130225 Firefox/22.0
Build ID: 20130225031141

Steps to reproduce:

Navigate to http://cmx.io/edit/


Actual results:

CPU utilization at 25%+ (quad-core system)
Slow to scroll
Version: 22 Branch → 19 Branch
Keywords: perf
SPS Profile against Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130226 Firefox/22.0 ID:20130226031002 CSet: 73f0c5b00572:

http://people.mozilla.com/~bgirard/cleopatra/#report=aa66c9becc9c5bb22ffcad848102f227aaf84ac1

-> GFX/SVG Jank

FWIW, the Site uses the RequireJS 2.0.5 (outdated) Library (http://github.com/jrburke/requirejs).
Component: Untriaged → Graphics
Product: Firefox → Core
Version: 19 Branch → Trunk
URL: http://cmx.io/edit/
OS: Windows 8 → All
Hardware: x86_64 → All
Summary: High CPU Utilization on Specific Page → High CPU Utilization on cmx.io
Status: UNCONFIRMED → NEW
Ever confirmed: true
Some additional information:

Using the paint-flashing tool recently released in nightly you can see that its constantly repainting the comic panels.

The repainting seems to be tied to this part of the editor code:

https://github.com/darwin/cmx.js/blob/master/app/edit/scripts/editor.coffee#L165

      set.css("min-height", "1000px").scrollTop(oldScrollTop);
      setInterval ->
        set.css("min-height", "")
      , 200

Its unclear to me why you would want to clear min-height every 200ms.  Is it supposed to force a repaint?  It doesn't seem to on Chrome.

Removing this setInterval() call stops the CPU spinning, though.

Profiling shows the majority of time is in the cairo font rendering.  This may just be a natural consequence of repainting every 200ms.
With a debug build the following is also generated on every repaint:

WARNING: Outer object paint value used outside SVG glyph: file /Users/bkelly/devel/hg/src/layout/svg/nsSVGGlyphFrame.cpp, line 1055
(That warning might be a red herring -- I've seen that elsewhere too.)
Here is a minimal case that triggers the pathological repainting of the <svg>
element:

<!DOCTYPE html>
<html>
  <head>
  </head>
  <body>
    <svg>
      <path d="M10 10 H 90 V 90 H 10 L 10 10"/>
    </svg>
    <script>
      var body = window.document.body;
      body.style.minHeight = '1px';
      setInterval(function() {
        body.style.minHeight = '';
      }, 1000);
    </script>
  </body>
</html>

Note, min-height is used in cmx and here, but it seems to effect any style
that is set to a value and then cleared.

Tracing the code shows that nsSVGUtils::ScheduleReflowSVG() is being called
unconditionally whenever DidSetStyleContext() is called on
nsSVGPathGeometryFrame and nsSVGGlyphFrame.  (nsSVGGlyphFrame is not
triggered by the example above, but is by the full cmx.io site.)

The attached patch is a naive attempt to address the problem in those two
methods.  With this applied I can view cmx.io/edit and the CPU stabalizes
around 10%.  Of course, there are other compatibility problems, so cmx.io
is still not fully functional.  Jetpack seems to pass with this patch
applied as well.

Alternatively, the following comment in nsStyleContext.h for
CalcStyleDifference suggests another issue could be at work:

   * Compute the style changes needed during restyling when this style
   * context is being replaced by aOther.  (This is nonsymmetric since
   * we optimize by skipping comparison for styles that have never been
   * requested.)

Perhaps there is an issue in the "skipping comparison for style that have
never been requested" logic when a style is cleared after having been
set once.  I have not been able track down this code yet.

I'm new to the codebase, so there is a good chance all of this is wrong.
Just to clarify, I think my question now is:

Should this kind of NOP style change be making it down to the SVG layer or should it be getting caught by a check further up the call stack?

Any guidance appreciated.  Thanks!
Currently nsDOMCSSDeclaration::RemoveProperty() short-circuits immediately if there is no underlying Declaration to operate on.  This patch extends this check to also short-circuit if you are removing a property from an empty Declaration.  This avoids the propagating the restyle event down to the SVG elements at all.

This patch still needs a unit test.  To be honest, its not clear to me yet which test-suite is the correct one for this kind of change.  I don't see make check in the new mach tool.
Attachment #725833 - Attachment is obsolete: true
Attachment #727045 - Attachment is patch: true
Attachment #725833 - Attachment is patch: true
[not commenting on the patch's correctness at this point -- I don't know precisely how nsDOMCSSDeclaration::RemoveProperty is supposed to work. I suspect bz might know if this is correct, though.  I can give it a TryServer run (a run of all our testsuites on all our supported platforms) to see if it breaks anything, too.] 

RE unit tests for this: So, automated performance testing is hard. :)  We use several test-suites collectively called "Talos" for per-checkin performance testing, but those tests are stored outside the mozilla-central source tree, and they don't tend to change much.  They're more broad tests of the general performance of different features, rather than specific unit tests for past perf issues.  So Talos probably isn't what you want here.

If this were a true hang (browser completely locks up), or if you can exploit this to *make* a testcase that effectively hangs, then you'd want to create a "crashtest".  The crashtest harness checks whether each of its tests load, and it times out after some threshold (I forget the exact threshold -- it's on the order of a few minutes, I think).

It may be tricky, though -- it'd need to load quickly (in seconds) in a patched build, because we don't want it to slow down our testsuite run-time too much, when the test is passing.  (That wouldn't scale well across many bugs)

So this might not be something we can usefully regression-test. :-/  Whoever ends up reviewing this might have thoughts on that too, though.

Feel free to ping me on IRC if you have more thoughts/questions about this.
(In reply to Daniel Holbert [:dholbert] from comment #9)
> I can give it a TryServer
> run (a run of all our testsuites on all our supported platforms) to see if
> it breaks anything, too.

I pushed attachment 727045 [details] [diff] [review] to Try: https://tbpl.mozilla.org/?tree=Try&rev=02779a4d736a
Hmm, I guess I was thinking of more of a micro unit test.  Specifically, a test case that creates a simple DOM, sets an attribute, and then clears that attribute multiple times.  The test would then check to see if the paint occurred on the second try using whatever API the paint-flashing tool uses.  Of course, I haven't investigated this yet so perhaps the hooks aren't there.

Are there any test suites with unit tests like that?  Perhaps this:

  https://developer.mozilla.org/en-US/docs/Compiled-code_automated_tests

Thanks again for your help!
Hmm.. I've actually never written one our C++ unit tests, and I'm not sure if they bring up enough of the browser to test things like painting.  I think those are  also generally are C++-only, rather than having C++ code combined with an HTML testcase.

Also: Paint-flashing doesn't really use an API to tell it whether a paint happened -- rather, it adds some instrumentation to the paint code itself, to modify what painting does[1]. So it's not something you can really hook into.

BUT: you definitely could use paint-flashing itself to tell you if a paint happened.  If it's enabled, you can compare two snapshots before and after a modification, and if they look different, then you know there was a paint.

To expand on this a bit -- you could write a mochitest that does something like:

 0) Turn on paint-flashing
 1) Load your helper-document, e.g. in an iframe. (not sure if an iframe would be required, or if it could just all happen in the same document -- it depends on whether that paint-flashing takes effect immediately or at load-time.)
 2) Wait for paints to flush
 3) take a snapshot
 4) Do you style-tweak that you're hoping won't trigger a paint
 5) Wait for any pending paints to flush
 6) take another snapshot
 7) Assert that your snapshots are identical
 8) Turn off paint-flashing (for the benefit of subsequent tests)

(Bonus points: Take an extra snapshot *before* you've enabled paint flashing, and compare it to the snapshot you take in step 3.  They should be different -- if they're not, then paint-flashing didn't get turned on, and you should make the test fail.  This will catch cases where paint-flashing isn't successfully being turned on, which would otherwise end up making your test trivially pass, without this bonus check.)

This should be possible using the "WindowSnapshot" library in our mochitest framework. (the snapshotWindow and compareSnapshots functions), and using "SpecialPowers" to turn on paint-flashing.

It looks like "test_bug847890_paintFlashing.html"[2] does most of these things already, so you could probably use it as a template.  Apparently that one is a "chrome mochitest"[3] (i.e. it has enhanced privileges) so yours probably needs to be one of those too, though I don't offhand know which of the APIs that it's using requires the enhanced privileges.  You could ask its author, or just ask in #developers, to find out, though.

[1] https://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#3353
[2] https://mxr.mozilla.org/mozilla-central/source/layout/base/tests/chrome/test_bug847890_paintFlashing.html?force=1
[3] https://developer.mozilla.org/en-US/docs/Chrome_tests
(The Try run from comment 10 is looking good -- so this doesn't seem to break any of our existing automated tests, for what that's worth!)
So my thoughts on the non-test bits of this:

1)  The SVG code that unconditionally reflows on any style context change is nuts.  If we
    don't have bugs on that already, we should.
2)  The attached patch is certainly safe, though I'd make the test "== 0" instead of "< 1"
3)  If desired, you could check whether the declaration has the property in question at
    all (via HasProperty; just have to watch out for shorthands) or have RemoveProperty
    return a boolean indicating whether anything was removed (this would still pay the
    cost for the EnsureMutable call, but would be cheaper than HasProperty in the
    hopefully-common case of a property that _is_ set being unset).

My gut feeling is we want the Count() == 0 check, but then also a "bail if RemoveProperty did nothing" similar to what we have to SetProperty already.  We'll need to make RemoveProperty return a boolean indicating whether it removed something.
Another patch for discussion.  This implement the suggestions provided by bz in comment 14.  Thanks for the feedback.

Note, would it make sense to deallocate the css::Declaration when its count reaches zero?  I haven't had time to trace the life cycle yet.

I also still need to make the unit test and formulate this as a proper hg patch.  Traveling at the moment so time is scarce.
Attachment #727045 - Attachment is obsolete: true
> Note, would it make sense to deallocate the css::Declaration when its count reaches
> zero?  

Probably not particularly.
>+      removed = mOrder.RemoveElement(*p);

  removed = removed || mOrder.RemoveElement(*p);

        Attachment #728023 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 728245 [details] [diff] [review]
Fix logic within macro'd loop based on bz feedback.

>-void
>+bool
> Declaration::RemoveProperty(nsCSSProperty aProperty)
> {
>   nsCSSExpandedDataBlock data;
>   ExpandTo(&data);
>   NS_ABORT_IF_FALSE(!mData && !mImportantData, "Expand didn't null things out");
> 
>+  bool removed = false;
>+
>   if (nsCSSProps::IsShorthand(aProperty)) {
>     CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, aProperty) {
>       data.ClearLonghandProperty(*p);
>-      mOrder.RemoveElement(*p);
>+      removed = removed || mOrder.RemoveElement(*p);
>     }
>   } else {
>     data.ClearLonghandProperty(aProperty);
>-    mOrder.RemoveElement(aProperty);
>+    removed = mOrder.RemoveElement(aProperty);
>   }
> 
>   CompressFrom(&data);
>+
>+  return removed;
> }

So in terms of the long term plans for this code, this patch seems a bit awkward to me, since |data| is the primary data structure and |mOrder| is a secondary structure that we'd like to remove in the future (see, say, bug 553456 / bug 649145).  So I'd rather get this data by having a boolean return value from data.ClearLonghandProperty and propagating that boolean than looking at the return value of mOrder.RemoveElement.

Otherwise, though, this seems like a fine patch... but we should also fix the wacky SVG code too.

    
    

    
  
(Component here should probably be either Style System or SVG, not Graphics.)
Component: Graphics → Style System (CSS)
Summary: High CPU Utilization on cmx.io → High CPU Utilization on cmx.io due to no-op removeProperty triggering excessive DidSetStyleContext handling on SVG frames

    
    

    
  
And to be clear:  the SVG code is wacky because nearly all users of the DidSetStyleContext hook are wacky; our primary style change handling codepath is the change hint processing loop in nsCSSFrameConstructor::ProcessRestyledFrames .  (I still need to write the part of https://wiki.mozilla.org/Gecko:Overview describing that, though.)

    
    

    
  


  
Updated patch based on dbaron's feedback regarding mOrder being a secondary data structure.

This required refactoring a bit more to push the bool return logic down into nsCSSExpandedDataBlock and nsCSSPropertySet.

While there I also refactored the code to avoid duplicating the shorthand property loop in css::Declaration.  Now that we are reporting when a property is cleared we can uniquely identify the exact property from nsCSSExpandedDataBlock::ClearProperty().  This is a slight pessimization for the ClearProperty() case where you don't care which property matched, but I'm guessing removing CSS properties is not a hot path.  (Should this sort of thing be placed in a separate patch?)

        Attachment #728245 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 728831 [details] [diff] [review]
Do not reset the style context when removing unset properties.

>-void
>+bool
> Declaration::RemoveProperty(nsCSSProperty aProperty)
> {
>   nsCSSExpandedDataBlock data;
>   ExpandTo(&data);
>   NS_ABORT_IF_FALSE(!mData && !mImportantData, "Expand didn't null things out");
> 
>-  if (nsCSSProps::IsShorthand(aProperty)) {
>-    CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, aProperty) {
>-      data.ClearLonghandProperty(*p);
>-      mOrder.RemoveElement(*p);
>-    }
>-  } else {
>-    data.ClearLonghandProperty(aProperty);
>-    mOrder.RemoveElement(aProperty);
>-  }
>+  nsCSSProperty clearedPropID;
>+  bool removed = data.ClearProperty(aProperty, clearedPropID);
>+  mOrder.RemoveElement(clearedPropID);
> 
>   CompressFrom(&data);
>+
>+  return removed;
> }

While this structure change works for ClearLonghangProperty/ClearProperty, it doesn't work for mOrder.RemoveElement(), since the loop can remove multiple properties.  It's probably best to switch back to the old structure and ensure that the boolean is handled properly in the loop.

>diff --git a/layout/style/nsCSSDataBlock.cpp b/layout/style/nsCSSDataBlock.cpp
>-void
>-nsCSSExpandedDataBlock::ClearProperty(nsCSSProperty aPropID)
>+bool
>+nsCSSExpandedDataBlock::ClearProperty(nsCSSProperty aPropID, nsCSSProperty &aReturn)
> {
>+  bool cleared = false;
>+
>   if (nsCSSProps::IsShorthand(aPropID)) {
>     CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, aPropID) {
>-      ClearLonghandProperty(*p);
>+      cleared = ClearLonghandProperty(*p);
>+      if (cleared) {
>+        aReturn = *p;
>+        break;
>+      }
>     }
>   } else {
>-    ClearLonghandProperty(aPropID);
>+    cleared = ClearLonghandProperty(aPropID);
>+    if (cleared) {
>+      aReturn = aPropID;
>+    }
>   }
>+
>+  return cleared;
> }

Then you can drop these changes (which break the  full clearing of a shorthand).

>diff --git a/layout/style/nsCSSDataBlock.h b/layout/style/nsCSSDataBlock.h
>--- a/layout/style/nsCSSDataBlock.h
>+++ b/layout/style/nsCSSDataBlock.h
>@@ -217,23 +217,31 @@ public:
>     /**
>      * Clear the state of this expanded block.
>      */
>     void Clear();
> 
>     /**
>      * Clear the data for the given property (including the set and
>      * important bits).  Can be used with shorthand properties.
>+     *
>+     * Returns true if a property was removed, false otherwise.  To determine
>+     * the exact property ID removed, pass a reference to an nsCSSProperty as
>+     * the second argument.
>      */
>-    void ClearProperty(nsCSSProperty aPropID);
>+    inline bool ClearProperty(nsCSSProperty aPropID) {
>+      nsCSSProperty clearedPropID;
>+      return ClearProperty(aPropID, clearedPropID);
>+    }
>+    bool ClearProperty(nsCSSProperty aPropID, nsCSSProperty &aReturn);
> 

And these changes.

    
    

    
  
Thanks for the clarification.  The short-circuit in comment 17 led me to believe only a single property would match and be removed in the loop.  Sorry for my confusion.

I'll fix the patch as you suggest.

    
    

    
  


  
Updated patch based on dbaron's feedback.

Also, I went and read up CSS shorthand properties so I have better idea of what is going on here.  I had previously been incorrectly thinking of these as a list of property aliases that mapped to a single longhand property.

        Attachment #728831 -
        Attachment is obsolete: true

    
    

    
  
Also, I couldn't find another bug referencing the SVG DidSetStyleContext issue, so I went ahead and opened bug 854765.

    
    

    
  


  
Updated patch with mochitest test case.

Note, I was unable to directly follow the test approach outlined in comment 12.  Unfortunately paint flashing appears to always be triggered by snapshotWindow() regardless of content changes.  This makes sense given that the snapshot is implemented by painting the document content into a new Canvas element.

This suggests that the existing test_bug847890_paintFlashing.html test case is not quite correct.  It attempts to show that paint flashing occurs in response to content changes, but in reality the paint flashing will always occur with this method.  See bug 856499.

To avoid this problem this mochitest instead listens for the "MozAfterPaint" event in order to determine if a paint occurred.

        Attachment #729375 -
        Attachment is obsolete: true

    
    

    
  
(Yikes! Good catch, and thanks for filing a bug on that test being broken.  Listening for MozAfterPaint sounds reasonable, anyway.)

    
    

    
  
Comment on attachment 731748 [details] [diff] [review]
Do not reset the style context when removing unset properties.

sorry for taking so long to get to this (I think I was ignoring the request since it came from me, which was a bad idea)

I suspect now that bug 854765 is fixed, this test won't test anything useful anymore.  Maybe I'm wrong, though.  But I'm inclined to think that this doesn't need an automated regression test; as a performance fix it tends to be hard to test, and a test that tests that it does not trigger something in particular might lose its effectiveness because of some other optimization (as I suspect already happened here).  I think if we wanted to test it, we'd want something that more directly measured whether we were rebuilding style data, such as a clearable boolean in nsDOMWindowUtils that was set to true by any call to RestyleTracker::DoProcessRestyles.

Are you interested in writing that test, or should we just land this as-is?  (Or both, I suppose?)


I was about to mark r=dbaron, and then I noticed one serious problem, though:

>   decl = decl->EnsureMutable();
>-  decl->RemoveProperty(aPropID);
>+  if (!decl->RemoveProperty(aPropID)) {
>+    return NS_OK; // nothing removed
>+  }
>+
>   return SetCSSDeclaration(decl);
> }

This is going to do bad things if decl->EnsureMutable() returned a new declaration.  I need to look into this further...

    
    

    
  
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #29)
> Are you interested in writing that test, or should we just land this as-is? 
> (Or both, I suppose?)

Unfortunately I don't have a lot of spare time at the moment to work on a new test.  If you think its appropriate I would be ok proceeding without a test.

> I was about to mark r=dbaron, and then I noticed one serious problem, though:
> 
> >   decl = decl->EnsureMutable();
> >-  decl->RemoveProperty(aPropID);
> >+  if (!decl->RemoveProperty(aPropID)) {
> >+    return NS_OK; // nothing removed
> >+  }
> >+
> >   return SetCSSDeclaration(decl);
> > }
> 
> This is going to do bad things if decl->EnsureMutable() returned a new
> declaration.  I need to look into this further...

Yes, you are correct.

Looking at Declaration::EnsureMutable(), it appears a new heap allocated Declaration is created without any reference counting.  The old Declaration is not touched at this point.

So I think something like the following might work:

  Declaration *mutableDecl = decl->EnsureMutable();
  if (!mutableDecl->RemoveProperty(aPropID)) {
    if (mutableDecl != decl) {
      delete mutableDecl;
    }
    return NS_OK;
  }

  return SetCSSDeclaration(mutableDecl);

What do you think?  Obviously I am not too familiar with side effects and whatnot in the style code.  :-\
Flags: needinfo?(dbaron)

    
    

    
  
(In reply to Ben Kelly [:bkelly] from comment #30)
> So I think something like the following might work:
> 
>   Declaration *mutableDecl = decl->EnsureMutable();
>   if (!mutableDecl->RemoveProperty(aPropID)) {
>     if (mutableDecl != decl) {
>       delete mutableDecl;
>     }
>     return NS_OK;
>   }
> 
>   return SetCSSDeclaration(mutableDecl);
> 
> What do you think?  Obviously I am not too familiar with side effects and
> whatnot in the style code.  :-\

Yeah, I think this is probably the best thing to do.  It's possible there's something that's more efficient in the face of repeated no-op mutation, but I'm not up to demonstrating that it's correct in a reasonable period of time.
Flags: needinfo?(dbaron)

    
    

    
  
Comment on attachment 731748 [details] [diff] [review]
Do not reset the style context when removing unset properties.

so r=dbaron with the revisions described in comment 29 and comment 30

        Attachment #731748 -
        Flags: review?(dbaron) → review+

    
  
Whiteboard: [ c= p=1 s=2013.07.26 ]

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Updated patch.
        — 
          — Splinter Review
      

    


  
Updated patch based on review comments.  Carrying r+ forward from comment 32.

Try build:

  https://tbpl.mozilla.org/?tree=Try&rev=93d8f2cde617
Assignee: nobody → bkelly

        Attachment #731748 -
        Attachment is obsolete: true
Status: NEW → ASSIGNED

        Attachment #777122 -
        Flags: review+

    
    

    
  
Linux and windows have completed green.  Mac is still running, but looking good.  Going to go ahead and flag this for checkin.
Keywords: checkin-needed

    
    

    
  
I backed out this patch on mozilla-inbound because it appears that it may have increased the number of assertions in the logs. The increase in assertions caused the logs to go beyond their size limit and then the browser-chrome test suite turned red.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fbd705d2328

Please verify that no new assertions were introduced by this patch and reland.

    
    

    
  
Oh, and the interesting characteristic was that those failures happened on 32-bit debug browser-chrome, but not on 64-bit debug browser-chrome.

    
    

    
  
I guess it was unwise of me to do a non-debug try run.  Here is another try with debug this time:

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

Trying to see if I can reproduce locally as well.

    
    

    
  
It appears this causes bug 761848 to trigger much more often with:

  ###!!! ASSERTION: Shouldn't be trying to restyle non-elements directly: '!aContent || aContent->IsElement()', file layout/base/nsStyleChangeList.cpp, line 65

With this patch we get that assertion over 2000 times in the browser chrome test.  After Jared's backout we only triggered the assertion 200 times during the test.
Depends on: 761848

    
  
Whiteboard: [ c= p=1 s=2013.07.26 ] → [ c= p=3 s=2013.07.26 ]

    
    

    
  
Actually, bug 761848 is talking about a specific test case.

It does seem we get these assertions normally through-out the test in a variety of test cases.  I don't see a separate bug for that.
No longer depends on: 761848

    
    

    
  
I'm hoping bug 828312 will fix some occurrences of that assertion.  But if it doesn't, then there are likely a bunch of different causes.

That said, I think there's something wrong here -- or at least something getting miscompiled by all the 32-bit compilers.  The bit-arithmetic looks ok, but I might be missing something.

    
    

    
  
Since nothing was jumping out at me as to the cause, I decided to experiment with some additional changes.  First, I tried the following to see if the problem was due to additional logic in RemoveProperty() used to detect the removal:

  https://hg.mozilla.org/try/rev/c3ea71de3641
  https://tbpl.mozilla.org/?tree=Try&rev=d99fc4889b53

This passed, which suggests that the assertions are caused by not calling SetCSSDeclaration().

To investigate further I added a boolean argument to SetCSSDeclaration() indicating if a change actually occurred or not.  I then experimented with short circuiting the different implementations of SetCSSDeclaration().

This showed that the failure occured by short-circuiting nsDOMCSSAttrDeclaration::SetCSSDeclaration() produces the assertions:

  https://hg.mozilla.org/try/rev/495e4dcb7b0b
  https://tbpl.mozilla.org/?tree=Try&rev=5747350dd0b8

Then finally I believe this shows that its safe to short-circuit SetCSSDeclaration() if mIsSMILOverride is true and the assertions occur if mIsSMILOverride is false.

  https://hg.mozilla.org/try/rev/9d66f4d911a5
  https://tbpl.mozilla.org/?tree=Try&rev=a4a23cc2e3ef

This change is essentially the same as the previous, but with the added condition of nsIsSMILOverride in order to short circuit nsDOMCSSAttrDeclaration::SetCSSDeclaration().

This in turn makes me wonder if the assertions are related to this:

  // XXXbz this is a bit of a hack, especially doing it before the
  // BeginUpdate(), but this is a good chokepoint where we know we
  // plan to modify the CSSDeclaration, so need to notify
  // AttributeWillChange if this is inline style.
  if (!mIsSMILOverride) {
    nsNodeUtils::AttributeWillChange(mElement, kNameSpaceID_None,
                                     nsGkAtoms::style,
                                     nsIDOMMutationEvent::MODIFICATION);
  }

at:

  http://mxr.mozilla.org/mozilla-central/source/layout/style/nsDOMCSSAttrDeclaration.cpp#94

This of course gets triggered by the call to DocToUpdate() in nsDOMDeclaration::CSSRemoveProperty().

Does this make any sense?  I thought I would report back before continuing to investigate.
Flags: needinfo?(dbaron)

    
    

    
  
(In reply to Ben Kelly [:bkelly] from comment #42)
> Then finally I believe this shows that its safe to short-circuit
> SetCSSDeclaration() if mIsSMILOverride is true and the assertions occur if
> mIsSMILOverride is false.

That's no surprise; it's the 99% case vs. the 1% case.

> This in turn makes me wonder if the assertions are related to this:
> 
>   // XXXbz this is a bit of a hack, especially doing it before the
>   // BeginUpdate(), but this is a good chokepoint where we know we
>   // plan to modify the CSSDeclaration, so need to notify
>   // AttributeWillChange if this is inline style.
>   if (!mIsSMILOverride) {
>     nsNodeUtils::AttributeWillChange(mElement, kNameSpaceID_None,
>                                      nsGkAtoms::style,
>                                      nsIDOMMutationEvent::MODIFICATION);
>   }
> 
> at:
> 
>  
> http://mxr.mozilla.org/mozilla-central/source/layout/style/
> nsDOMCSSAttrDeclaration.cpp#94
> 
> This of course gets triggered by the call to DocToUpdate() in
> nsDOMDeclaration::CSSRemoveProperty().
> 
> Does this make any sense?  I thought I would report back before continuing
> to investigate.

I think it might.  I think bz might have a better idea of what, if anything, could go wrong as a result of calling AttributeWillChange without actually seting the attribute.

Though it's still not clear why the failures are happening on 32-bit but not 64-bit.  (Maybe something is wrong with the bit twiddling such that the patch behaves as intended on one but not the other?)
Flags: needinfo?(dbaron) → needinfo?(bzbarsky)

    
    

    
  
Hrm.  I don't see how an AttributeWillChange without a corresponding AttributeChanged would lead to the "restyling non-elements" assertions, exactly...  In fact, I just looked at the AttributeWillChange consumers, and except maybe for a11y all of them look like they can handle an AttributeWillChange that's not followed by an AttributeChanged.
Flags: needinfo?(bzbarsky)

    
    

    
  
But you could change directly: take out the firing of AttributeWillChange altogether.  It'll break some things, but not enough to make it impossible to run tests, I expect.

    
  
Whiteboard: [ c= p=3 s=2013.07.26 ] → [ c= p=3 s=2013.08.09 ]

    
    

    
  
Ok, I'll continue to investigate.  I've been trying to do this in the background using try builds, but I think I will work on reproducing locally.  Just need to figure out a 32-bit build setup since all my current environments are 64-bit.  Sorry for creating so much trouble with this bug.

    
    

    
  
Removed sprint identifier, updated points to 5 due to investigation time after discussion w/ dietrich and mlee. Please adjust back if needed.
Whiteboard: [ c= p=3 s=2013.08.09 ] → [ c= p=5 ]

    
    

    
  
Without having read this bug, and based purely on the "DidSetStyleContext handling on SVG frames" in the Summary line, this is fixed by the work I did to kill off the DidSetStyleContext overrides in the SVG code, no?

    
    

    
  
(In reply to Jonathan Watt [:jwatt] from comment #48)
> Without having read this bug, and based purely on the "DidSetStyleContext
> handling on SVG frames" in the Summary line, this is fixed by the work I did
> to kill off the DidSetStyleContext overrides in the SVG code, no?

The specific failure mode is fixed, yes.  Thank you!  I think there was still some thought that we could optimize and save some work if we detected that no attribute was removed.  Unfortunately I just haven't had time to investigate the test failures in some time.

    
    

    
  
Cool. I'd suggest that you close this as fixed by bug 854765, and open a follow-up for further optimization. Up to you though.
Depends on: 854765

    
    

    
  
Ben, should we go ahead and close this out per comment 50? If so, please go ahead and resolve/wontfix the bug.
Flags: needinfo?(bkelly)
Priority: -- → P4
Whiteboard: [ c= p=5 ] → [u= s= c= p=5 ]

    
    

    
  
(In reply to Geo Mealer [:geo] from comment #51)
> If so, please go ahead and resolve/wontfix the bug.

(not WONTFIX -- that implies we've decided not to fix the issue. Either FIXED [if we know for sure that bug 854765 fixed the problem] -- perhaps with the patch here obsoleted to make it clear that the patch didn't land -- or WORKSFORME [if the issue's gone but we don't know for sure why] would be more appropriate)

    
    

    
  
Unless someone strongly objects I'd like to keep this bug since it has the review history for the patch that landed and got backed out.  I'll change the description to match what this patch is trying to do.  I still intend to come back to this eventually.
No longer depends on: 854765
Flags: needinfo?(bkelly)
Summary: High CPU Utilization on cmx.io due to no-op removeProperty triggering excessive DidSetStyleContext handling on SVG frames → Do not reset the style context when removing unset properties

    
    

    
  
(In reply to Ben Kelly [:bkelly] from comment #53)
> Unless someone strongly objects I'd like to keep this bug since it has the
> review history for the patch that landed and got backed out.  I'll change
> the description to match what this patch is trying to do.  I still intend to
> come back to this eventually.

Agreed; that sounds like the right thing, unless someone else wants to finish it sooner.

    
    

    
  
It looks like this was fixed recently in bug 1435139.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: