Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
580373002:
[Invalidation Tracking] Trace StyleInvalidator setNeedsStyleRecalc (Closed)
|
Created:
6 years, 3 months ago by kouhei (in TOK) Modified:
6 years, 2 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, dglazkov+blink, ed+blinkwatch_opera.com, rwlbuis, rune+blink, tasak Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
Description[Invalidation Tracking] Trace StyleInvalidator setNeedsStyleRecalc
This patch adds a set of trace events to track reason when
StyleInvalidator invokes Node::setNeedsStyleRecalc. One of the trace events is guaranteed to be issued before StyleInvalidator issues setNeedsStyleRecalc on the node. The trace events come with InspectorStyleInvalidatorInvalidateEvent, which contains inspector node id, human readable reason string, and optionally the tagName/id/class/attr/invalidationList involved.
This is to be used from devtools so we post process and find the StyleInvalidator trace event on the given node to see the detailed reason for why StyleInvalidator decided issue setNeedsStyleRecalc on the node.
BUG=410701
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183177
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183643
Patch Set 1 #Patch Set 2 : wip #Patch Set 3 : working #Patch Set 4 : minimize diff #
Total comments: 18
Patch Set 5 : rebased #
Total comments: 13
Patch Set 6 : rebased #
Total comments: 6
Patch Set 7 : structured json #Patch Set 8 : Workaround regression by templates #Patch Set 9 : add missing files #
Total comments: 2
Patch Set 10 : UNLIKELY #Patch Set 11 : ALWAYS_INLINE #Patch Set 12 : track custom pseudo #
Total comments: 7
Patch Set 13 : use auto #Patch Set 14 : #Patch Set 15 : no ::init() #Patch Set 16 : revert change in StyleInvalidator.h #
Total comments: 2
Patch Set 17 : attributes #
Messages
Total messages: 39 (5 generated)
kouhei@chromium.org changed reviewers: + caseq@chromium.org, pdr@chromium.org
I think this patch is ready for review now. Would you take a look?
https://codereview.chromium.org/580373002/diff/60001/Source/core/css/invalida... File Source/core/css/invalidation/DescendantInvalidationSet.h (right): https://codereview.chromium.org/580373002/diff/60001/Source/core/css/invalida... Source/core/css/invalidation/DescendantInvalidationSet.h:78: String toString() const; Lets make it hard to use for anything else as a comment can be easy to miss :) String asTraceEventString() ? https://codereview.chromium.org/580373002/diff/60001/Source/core/css/invalida... File Source/core/css/invalidation/StyleInvalidator.cpp (right): https://codereview.chromium.org/580373002/diff/60001/Source/core/css/invalida... Source/core/css/invalidation/StyleInvalidator.cpp:114: TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("devtools.timeline.invalidationTracking"), Can you add a newline here so it's clear the comment and trace event are unrelated? https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorTraceEvents.cpp (right): https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:93: PassRefPtr<TraceEvent::ConvertableToTraceFormat> InspectorStyleInvalidatorInvalidateEvent::withExtraData(Element& element, const char* reason, const String& extraData) Can you use a default arg here to avoid needing two functions? ... char* reason, const String& extraData = nullAtom) { https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:96: value->setInteger("nodeId", InspectorNodeIds::idForNode(&element)); Can you use setNodeInfo here (from https://codereview.chromium.org/600733005) https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:98: value->setString("extraData", extraData); Can we be more descriptive than just "extraData" (here, and elsewhere in this patch)?
https://codereview.chromium.org/580373002/diff/60001/Source/core/css/invalida... File Source/core/css/invalidation/DescendantInvalidationSet.h (right): https://codereview.chromium.org/580373002/diff/60001/Source/core/css/invalida... Source/core/css/invalidation/DescendantInvalidationSet.h:78: String toString() const; On 2014/09/30 02:08:49, pdr wrote: > Lets make it hard to use for anything else as a comment can be easy to miss :) > > String asTraceEventString() ? The tricky point is that this is also used inside show(). https://codereview.chromium.org/580373002/diff/60001/Source/core/css/invalida... File Source/core/css/invalidation/StyleInvalidator.cpp (right): https://codereview.chromium.org/580373002/diff/60001/Source/core/css/invalida... Source/core/css/invalidation/StyleInvalidator.cpp:114: TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("devtools.timeline.invalidationTracking"), On 2014/09/30 02:08:49, pdr wrote: > Can you add a newline here so it's clear the comment and trace event are > unrelated? I think this comment is related. IIUC, theoretically we don't need to setNeedsStyleRecalcStyle here, as the element might not match the current invalidation set. This is forcing styleRecalc to disable RenderStyle sharing for the element. https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorTraceEvents.cpp (right): https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:93: PassRefPtr<TraceEvent::ConvertableToTraceFormat> InspectorStyleInvalidatorInvalidateEvent::withExtraData(Element& element, const char* reason, const String& extraData) On 2014/09/30 02:08:49, pdr wrote: > Can you use a default arg here to avoid needing two functions? > > ... char* reason, const String& extraData = nullAtom) { Acknowledged. https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:96: value->setInteger("nodeId", InspectorNodeIds::idForNode(&element)); On 2014/09/30 02:08:50, pdr wrote: > Can you use setNodeInfo here (from https://codereview.chromium.org/600733005) Acknowledged. https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:98: value->setString("extraData", extraData); On 2014/09/30 02:08:49, pdr wrote: > Can we be more descriptive than just "extraData" (here, and elsewhere in this > patch)? Agreed, but want your help on naming. "extraData" is currently used to contain tagName/class/id/pseudo name that caused invalidation. - context? - sourceName?
https://codereview.chromium.org/580373002/diff/60001/Source/core/css/invalida... File Source/core/css/invalidation/DescendantInvalidationSet.h (right): https://codereview.chromium.org/580373002/diff/60001/Source/core/css/invalida... Source/core/css/invalidation/DescendantInvalidationSet.h:78: String toString() const; On 2014/09/30 02:22:00, kouhei wrote: > On 2014/09/30 02:08:49, pdr wrote: > > Lets make it hard to use for anything else as a comment can be easy to miss :) > > > > String asTraceEventString() ? > > The tricky point is that this is also used inside show(). show() is just for debugging though and is not used in any shipping code. Alternatively, "asDebugString()"? https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorTraceEvents.cpp (right): https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:95: RefPtr<TracedValue> value = TracedValue::create(); We need the frame info here too. Can you add: value->setString("frame", toHexString(element->document().frame())); https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:98: value->setString("extraData", extraData); Talked offline a bit. We don't actually have the entire selector, only the last matched part. Something like selectorInfo or selectorPart will probably be best.
https://codereview.chromium.org/580373002/diff/60001/Source/core/css/invalida... File Source/core/css/invalidation/DescendantInvalidationSet.h (right): https://codereview.chromium.org/580373002/diff/60001/Source/core/css/invalida... Source/core/css/invalidation/DescendantInvalidationSet.h:78: String toString() const; On 2014/09/30 03:21:57, pdr wrote: > On 2014/09/30 02:22:00, kouhei wrote: > > On 2014/09/30 02:08:49, pdr wrote: > > > Lets make it hard to use for anything else as a comment can be easy to miss > :) > > > > > > String asTraceEventString() ? > > > > The tricky point is that this is also used inside show(). > > show() is just for debugging though and is not used in any shipping code. > Alternatively, "asDebugString()"? Done. https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorTraceEvents.cpp (right): https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:93: PassRefPtr<TraceEvent::ConvertableToTraceFormat> InspectorStyleInvalidatorInvalidateEvent::withExtraData(Element& element, const char* reason, const String& extraData) On 2014/09/30 02:22:00, kouhei wrote: > On 2014/09/30 02:08:49, pdr wrote: > > Can you use a default arg here to avoid needing two functions? > > > > ... char* reason, const String& extraData = nullAtom) { > > Acknowledged. Done. https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:95: RefPtr<TracedValue> value = TracedValue::create(); On 2014/09/30 03:21:57, pdr wrote: > We need the frame info here too. Can you add: > value->setString("frame", toHexString(element->document().frame())); Done. https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:96: value->setInteger("nodeId", InspectorNodeIds::idForNode(&element)); On 2014/09/30 02:22:00, kouhei wrote: > On 2014/09/30 02:08:50, pdr wrote: > > Can you use setNodeInfo here (from https://codereview.chromium.org/600733005) > > Acknowledged. Done. https://codereview.chromium.org/580373002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:98: value->setString("extraData", extraData); On 2014/09/30 03:21:57, pdr wrote: > Talked offline a bit. We don't actually have the entire selector, only the last > matched part. Something like selectorInfo or selectorPart will probably be best. Done.
LGTM. Please wait for caseq's LGTM as well.
yurys@chromium.org changed reviewers: + yurys@chromium.org
https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... File Source/core/inspector/InspectorTraceEvents.cpp (right): https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:70: StringBuilder builder; Please use TracedValue instead for building the string. You can pass instance created in InspectorStyleInvalidatorInvalidateEvent::data as a parameter and populate it with "selectorPart" array here. DescendantInvalidationSet::toDebugString could also be changed to toTracedValue that would write its output directly into a TracedValue.
https://codereview.chromium.org/580373002/diff/80001/Source/core/css/invalida... File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): https://codereview.chromium.org/580373002/diff/80001/Source/core/css/invalida... Source/core/css/invalidation/DescendantInvalidationSet.cpp:252: fprintf(stderr, "%s\n", toString().ascii().data()); toDebugString() perhaps? https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... File Source/core/inspector/InspectorTraceEvents.cpp (right): https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:70: StringBuilder builder; On 2014/09/30 09:21:14, yurys wrote: > Please use TracedValue instead for building the string. +1 to that. https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:102: return value; nit: value.release(), since you've just changed this elsewhere :-) https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... File Source/core/inspector/InspectorTraceEvents.h (right): https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.h:42: class InspectorStyleInvalidatorInvalidateEvent { nit: here and below, I wonder if we need both Invalidator and Invalidate in one identifier. InspectorStyleInvalidationEvent perhaps? https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.h:55: #define TRACE_STYLE_INVALIDATOR_INVALIDATION(ELEMENT, REASON) \ nit: plase add a blank line above the macro definition. Also, lowercase the macro parameters, they normally follow function arguments style. https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.h:61: #define TRACE_STYLE_INVALIDATOR_INVALIDATION_SELECTORPART(ELEMENT, REASON, SELECTORPART) \ ditto
I've changed it to use TracedValue instead. PTAL. https://codereview.chromium.org/580373002/diff/80001/Source/core/css/invalida... File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): https://codereview.chromium.org/580373002/diff/80001/Source/core/css/invalida... Source/core/css/invalidation/DescendantInvalidationSet.cpp:252: fprintf(stderr, "%s\n", toString().ascii().data()); On 2014/09/30 12:24:00, caseq wrote: > toDebugString() perhaps? Done. https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... File Source/core/inspector/InspectorTraceEvents.cpp (right): https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:70: StringBuilder builder; On 2014/09/30 12:24:00, caseq wrote: > On 2014/09/30 09:21:14, yurys wrote: > > Please use TracedValue instead for building the string. > +1 to that. Done. https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.cpp:102: return value; On 2014/09/30 12:24:00, caseq wrote: > nit: value.release(), since you've just changed this elsewhere :-) Done. https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... File Source/core/inspector/InspectorTraceEvents.h (right): https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.h:42: class InspectorStyleInvalidatorInvalidateEvent { On 2014/09/30 12:24:00, caseq wrote: > nit: here and below, I wonder if we need both Invalidator and Invalidate in one > identifier. InspectorStyleInvalidationEvent perhaps? . I think StyleInvalidatorInvalidate is correct here. This is recalcStyle invalidation caused by StyleInvalidator, which precedes InspectorStyleRecalcInvalidationTrackingEvent. https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.h:55: #define TRACE_STYLE_INVALIDATOR_INVALIDATION(ELEMENT, REASON) \ On 2014/09/30 12:24:00, caseq wrote: > nit: plase add a blank line above the macro definition. Also, lowercase the > macro parameters, they normally follow function arguments style. Done. https://codereview.chromium.org/580373002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorTraceEvents.h:61: #define TRACE_STYLE_INVALIDATOR_INVALIDATION_SELECTORPART(ELEMENT, REASON, SELECTORPART) \ On 2014/09/30 12:24:00, caseq wrote: > ditto Done.
https://codereview.chromium.org/580373002/diff/100001/Source/core/css/invalid... File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): https://codereview.chromium.org/580373002/diff/100001/Source/core/css/invalid... Source/core/css/invalidation/DescendantInvalidationSet.cpp:212: value->pushString("*"); Can we please get it more structured -- i.e. instead of custom-encoded string, use a JSON object? I realize current format is tailored after show() where it was more human-readable, but now that we expose this thorough DevTools, I think we should make it more convenient to process programmatically (and I hope that once this is supported in DevTools, the debug output will no longer be necessary). https://codereview.chromium.org/580373002/diff/100001/Source/core/inspector/I... File Source/core/inspector/InspectorTraceEvents.cpp (right): https://codereview.chromium.org/580373002/diff/100001/Source/core/inspector/I... Source/core/inspector/InspectorTraceEvents.cpp:96: value->pushString(selectorPart); So selectorParts contents is either a string (here) or arrays (below). Can we make it more strongly typed (I guess the transition to objects in DescendentInvalidationSet::toTracedValue() should help, just push an object like { selectedPart: ... } here)? https://codereview.chromium.org/580373002/diff/100001/Source/core/inspector/I... Source/core/inspector/InspectorTraceEvents.cpp:105: for (const auto& invalidationSet : invalidationList) { nit: redundant {}
Thanks for your comments. PTAL https://codereview.chromium.org/580373002/diff/100001/Source/core/css/invalid... File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): https://codereview.chromium.org/580373002/diff/100001/Source/core/css/invalid... Source/core/css/invalidation/DescendantInvalidationSet.cpp:212: value->pushString("*"); On 2014/10/01 07:50:49, caseq wrote: > Can we please get it more structured -- i.e. instead of custom-encoded string, > use a JSON object? I realize current format is tailored after show() where it > was more human-readable, but now that we expose this thorough DevTools, I think > we should make it more convenient to process programmatically (and I hope that > once this is supported in DevTools, the debug output will no longer be > necessary). Done. https://codereview.chromium.org/580373002/diff/100001/Source/core/inspector/I... File Source/core/inspector/InspectorTraceEvents.cpp (right): https://codereview.chromium.org/580373002/diff/100001/Source/core/inspector/I... Source/core/inspector/InspectorTraceEvents.cpp:96: value->pushString(selectorPart); On 2014/10/01 07:50:50, caseq wrote: > So selectorParts contents is either a string (here) or arrays (below). Can we > make it more strongly typed (I guess the transition to objects in > DescendentInvalidationSet::toTracedValue() should help, just push an object like > { selectedPart: ... } here)? I changed this to use different key for each. https://codereview.chromium.org/580373002/diff/100001/Source/core/inspector/I... Source/core/inspector/InspectorTraceEvents.cpp:105: for (const auto& invalidationSet : invalidationList) { On 2014/10/01 07:50:50, caseq wrote: > nit: redundant {} Done.
lgtm
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/580373002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as 183177
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/624133003/ by kouhei@chromium.org. The reason for reverting is: Regressed blink_perf.css.
Here is a second attempt to land this. Would you take a look at patchset 8? I locally confirmed that patchset 8 doesn't regress performance. Sorry that I ended up with templates, these functions seems to be so performance sensitive that adding a if stmt regresses performance by 0.5% each. I first tried caching TRACE_EVENT_API_GET_CATEGORY_ENABLED result, but it wasn't enough.
According to tasak@, using templates to avoid if statement is already common in css code. e.g.) https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... However alternative solutions are welcome.
pdr: friendly ping. Please review this when you have time.
I apologize for the review delay. https://codereview.chromium.org/580373002/diff/160001/Source/core/css/invalid... File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): https://codereview.chromium.org/580373002/diff/160001/Source/core/css/invalid... Source/core/css/invalidation/DescendantInvalidationSet.cpp:49: template <StyleInvalidationTracingEnabledFlag styleInvalidationTracingEnabled> This function is hot and we want it to be as fast as possible. At the same time, we also need it to be as maintainable as possible for the first goal :) I think we can do a little better in the second case. Can we use the approach taken in GraphicsContextAnnotator? It sets a bit in ContentLayerDelegate for whether the trace is enabled and hides the bit check behind the ANNOTATE_GRAPHICS_CONTEXT macro.
https://codereview.chromium.org/580373002/diff/160001/Source/core/css/invalid... File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): https://codereview.chromium.org/580373002/diff/160001/Source/core/css/invalid... Source/core/css/invalidation/DescendantInvalidationSet.cpp:49: template <StyleInvalidationTracingEnabledFlag styleInvalidationTracingEnabled> On 2014/10/10 04:36:07, pdr wrote: > This function is hot and we want it to be as fast as possible. At the same time, > we also need it to be as maintainable as possible for the first goal :) I think > we can do a little better in the second case. > > Can we use the approach taken in GraphicsContextAnnotator? It sets a bit in > ContentLayerDelegate for whether the trace is enabled and hides the bit check > behind the ANNOTATE_GRAPHICS_CONTEXT macro. I tried a similar approach but performance regression was still 4-5%. It looks like any additional dynamic "if" stmts inside this function cause significant regression.
On 2014/10/10 at 07:15:07, kouhei wrote: > https://codereview.chromium.org/580373002/diff/160001/Source/core/css/invalid... > File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): > > https://codereview.chromium.org/580373002/diff/160001/Source/core/css/invalid... > Source/core/css/invalidation/DescendantInvalidationSet.cpp:49: template <StyleInvalidationTracingEnabledFlag styleInvalidationTracingEnabled> > On 2014/10/10 04:36:07, pdr wrote: > > This function is hot and we want it to be as fast as possible. At the same time, > > we also need it to be as maintainable as possible for the first goal :) I think > > we can do a little better in the second case. > > > > Can we use the approach taken in GraphicsContextAnnotator? It sets a bit in > > ContentLayerDelegate for whether the trace is enabled and hides the bit check > > behind the ANNOTATE_GRAPHICS_CONTEXT macro. > > I tried a similar approach but performance regression was still 4-5%. It looks like any additional dynamic "if" stmts inside this function cause significant regression. Did you try using UNLIKELY as well? Fmalita added the graphics context annotations and they are in most paint calls which are similarly hot.
On 2014/10/11 at 03:16:18, pdr wrote: > On 2014/10/10 at 07:15:07, kouhei wrote: > > https://codereview.chromium.org/580373002/diff/160001/Source/core/css/invalid... > > File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): > > > > https://codereview.chromium.org/580373002/diff/160001/Source/core/css/invalid... > > Source/core/css/invalidation/DescendantInvalidationSet.cpp:49: template <StyleInvalidationTracingEnabledFlag styleInvalidationTracingEnabled> > > On 2014/10/10 04:36:07, pdr wrote: > > > This function is hot and we want it to be as fast as possible. At the same time, > > > we also need it to be as maintainable as possible for the first goal :) I think > > > we can do a little better in the second case. > > > > > > Can we use the approach taken in GraphicsContextAnnotator? It sets a bit in > > > ContentLayerDelegate for whether the trace is enabled and hides the bit check > > > behind the ANNOTATE_GRAPHICS_CONTEXT macro. > > > > I tried a similar approach but performance regression was still 4-5%. It looks like any additional dynamic "if" stmts inside this function cause significant regression. > > Did you try using UNLIKELY as well? Fmalita added the graphics context annotations and they are in most paint calls which are similarly hot. Ping. I don't want to unnecessarily hold this patch up, but I think trying with UNLIKELY() is worth the effort given the performance regression we're looking at.
On 2014/10/13 21:14:29, pdr wrote: > On 2014/10/11 at 03:16:18, pdr wrote: > > On 2014/10/10 at 07:15:07, kouhei wrote: > > > > https://codereview.chromium.org/580373002/diff/160001/Source/core/css/invalid... > > > File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): > > > > > > > https://codereview.chromium.org/580373002/diff/160001/Source/core/css/invalid... > > > Source/core/css/invalidation/DescendantInvalidationSet.cpp:49: template > <StyleInvalidationTracingEnabledFlag styleInvalidationTracingEnabled> > > > On 2014/10/10 04:36:07, pdr wrote: > > > > This function is hot and we want it to be as fast as possible. At the same > time, > > > > we also need it to be as maintainable as possible for the first goal :) I > think > > > > we can do a little better in the second case. > > > > > > > > Can we use the approach taken in GraphicsContextAnnotator? It sets a bit > in > > > > ContentLayerDelegate for whether the trace is enabled and hides the bit > check > > > > behind the ANNOTATE_GRAPHICS_CONTEXT macro. > > > > > > I tried a similar approach but performance regression was still 4-5%. It > looks like any additional dynamic "if" stmts inside this function cause > significant regression. > > > > Did you try using UNLIKELY as well? Fmalita added the graphics context > annotations and they are in most paint calls which are similarly hot. > > Ping. I don't want to unnecessarily hold this patch up, but I think trying with > UNLIKELY() is worth the effort given the performance regression we're looking > at. Sorry. It was a national holiday yesterday. I'll give UNLIKELY a try now.
On 2014/10/13 23:54:02, kouhei wrote: > On 2014/10/13 21:14:29, pdr wrote: > > On 2014/10/11 at 03:16:18, pdr wrote: > > > On 2014/10/10 at 07:15:07, kouhei wrote: > > > > > > > https://codereview.chromium.org/580373002/diff/160001/Source/core/css/invalid... > > > > File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): > > > > > > > > > > > https://codereview.chromium.org/580373002/diff/160001/Source/core/css/invalid... > > > > Source/core/css/invalidation/DescendantInvalidationSet.cpp:49: template > > <StyleInvalidationTracingEnabledFlag styleInvalidationTracingEnabled> > > > > On 2014/10/10 04:36:07, pdr wrote: > > > > > This function is hot and we want it to be as fast as possible. At the > same > > time, > > > > > we also need it to be as maintainable as possible for the first goal :) > I > > think > > > > > we can do a little better in the second case. > > > > > > > > > > Can we use the approach taken in GraphicsContextAnnotator? It sets a bit > > in > > > > > ContentLayerDelegate for whether the trace is enabled and hides the bit > > check > > > > > behind the ANNOTATE_GRAPHICS_CONTEXT macro. > > > > > > > > I tried a similar approach but performance regression was still 4-5%. It > > looks like any additional dynamic "if" stmts inside this function cause > > significant regression. > > > > > > Did you try using UNLIKELY as well? Fmalita added the graphics context > > annotations and they are in most paint calls which are similarly hot. > > > > Ping. I don't want to unnecessarily hold this patch up, but I think trying > with > > UNLIKELY() is worth the effort given the performance regression we're looking > > at. > > Sorry. It was a national holiday yesterday. I'll give UNLIKELY a try now. Using UNLIKELY + ALWAYS_INLINE made this to 0 regression. PTAL.
Just a few minor questions https://codereview.chromium.org/580373002/diff/280001/Source/core/Init.cpp File Source/core/Init.cpp (right): https://codereview.chromium.org/580373002/diff/280001/Source/core/Init.cpp#ne... Source/core/Init.cpp:106: DescendantInvalidationSet::init(); // Depends on EventTracer::initialize() Are these still needed? https://codereview.chromium.org/580373002/diff/280001/Source/core/css/invalid... File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): https://codereview.chromium.org/580373002/diff/280001/Source/core/css/invalid... Source/core/css/invalidation/DescendantInvalidationSet.cpp:87: for (WillBeHeapHashSet<AtomicString>::const_iterator it = m_attributes->begin(); it != m_attributes->end(); ++it) { Nit: Is there a reason you can't use auto here? https://codereview.chromium.org/580373002/diff/280001/Source/core/css/invalid... File Source/core/css/invalidation/StyleInvalidator.cpp (right): https://codereview.chromium.org/580373002/diff/280001/Source/core/css/invalid... Source/core/css/invalidation/StyleInvalidator.cpp:98: if (UNLIKELY(*s_tracingEnabled)) Can you break this unlikely out into a macro using the same _IF_ENABLED pattern?
https://codereview.chromium.org/580373002/diff/280001/Source/core/Init.cpp File Source/core/Init.cpp (right): https://codereview.chromium.org/580373002/diff/280001/Source/core/Init.cpp#ne... Source/core/Init.cpp:106: DescendantInvalidationSet::init(); // Depends on EventTracer::initialize() On 2014/10/14 01:36:47, pdr wrote: > Are these still needed? Yes. These caches pointers to tracing flag. Accounts for 1-2% of overhead. https://codereview.chromium.org/580373002/diff/280001/Source/core/css/invalid... File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): https://codereview.chromium.org/580373002/diff/280001/Source/core/css/invalid... Source/core/css/invalidation/DescendantInvalidationSet.cpp:87: for (WillBeHeapHashSet<AtomicString>::const_iterator it = m_attributes->begin(); it != m_attributes->end(); ++it) { On 2014/10/14 01:36:47, pdr wrote: > Nit: Is there a reason you can't use auto here? This was unintended from failed rebase. Thanks for the catch. https://codereview.chromium.org/580373002/diff/280001/Source/core/css/invalid... File Source/core/css/invalidation/StyleInvalidator.cpp (right): https://codereview.chromium.org/580373002/diff/280001/Source/core/css/invalid... Source/core/css/invalidation/StyleInvalidator.cpp:98: if (UNLIKELY(*s_tracingEnabled)) On 2014/10/14 01:36:47, pdr wrote: > Can you break this unlikely out into a macro using the same _IF_ENABLED pattern? Done.
https://codereview.chromium.org/580373002/diff/280001/Source/core/Init.cpp File Source/core/Init.cpp (right): https://codereview.chromium.org/580373002/diff/280001/Source/core/Init.cpp#ne... Source/core/Init.cpp:106: DescendantInvalidationSet::init(); // Depends on EventTracer::initialize() On 2014/10/14 02:02:10, kouhei wrote: > On 2014/10/14 01:36:47, pdr wrote: > > Are these still needed? > > Yes. These caches pointers to tracing flag. Accounts for 1-2% of overhead. Discussed offline. I changed these flags to be initialized inside StyleInvalidator c-tor.
On 2014/10/14 at 04:18:14, kouhei wrote: > https://codereview.chromium.org/580373002/diff/280001/Source/core/Init.cpp > File Source/core/Init.cpp (right): > > https://codereview.chromium.org/580373002/diff/280001/Source/core/Init.cpp#ne... > Source/core/Init.cpp:106: DescendantInvalidationSet::init(); // Depends on EventTracer::initialize() > On 2014/10/14 02:02:10, kouhei wrote: > > On 2014/10/14 01:36:47, pdr wrote: > > > Are these still needed? > > > > Yes. These caches pointers to tracing flag. Accounts for 1-2% of overhead. > > Discussed offline. I changed these flags to be initialized inside StyleInvalidator c-tor. LGTM
kouhei@chromium.org changed reviewers: + haraken@chromium.org, tasak@google.com
tasak,haraken: Would you do a second pass on this CL? This patch is touching performance critical areas so I'd appreciate it if you would take a look.
lgtm https://codereview.chromium.org/580373002/diff/360001/Source/core/css/invalid... File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): https://codereview.chromium.org/580373002/diff/360001/Source/core/css/invalid... Source/core/css/invalidation/DescendantInvalidationSet.cpp:248: value->beginArray("ids"); Nit: attributes?
https://codereview.chromium.org/580373002/diff/360001/Source/core/css/invalid... File Source/core/css/invalidation/DescendantInvalidationSet.cpp (right): https://codereview.chromium.org/580373002/diff/360001/Source/core/css/invalid... Source/core/css/invalidation/DescendantInvalidationSet.cpp:248: value->beginArray("ids"); On 2014/10/14 04:35:53, tasak wrote: > Nit: attributes? Indeed. Thanks for the catch!
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/580373002/380001
Message was sent while issue was closed.
Committed patchset #17 (id:380001) as 183643 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
