Bug 91759 – Web Inspector: count RenderStyle objects in the native memory profiler

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Alexei Filippov]

EOM

[Alexei Filippov]

Created attachment 153306 [details]
Patch

[Early Warning System Bot]

Comment on attachment 153306 [details]
Patch

Attachment 153306 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13303004

[Build Bot]

Comment on attachment 153306 [details]
Patch

Attachment 153306 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13281981

[Build Bot]

Comment on attachment 153306 [details]
Patch

Attachment 153306 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13274920

[Early Warning System Bot]

Comment on attachment 153306 [details]
Patch

Attachment 153306 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13284949

[Gyuyoung Kim]

Comment on attachment 153306 [details]
Patch

Attachment 153306 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13303036

[Pavel Feldman]

Comment on attachment 153306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153306&action=review

> Source/WebCore/rendering/style/RenderStyle.cpp:1492
> +    MemoryClassInfo<RenderStyle> info(memoryObjectInfo, this, MemoryInstrumentation::CSS);

Please do not submit additional instrumentation until we find a way to maintain it.

[Alexei Filippov]

Created attachment 153471 [details]
Patch

[Alexei Filippov]

Created attachment 153479 [details]
Patch

[Alexei Filippov]

Comment on attachment 153306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153306&action=review

>> Source/WebCore/rendering/style/RenderStyle.cpp:1492
>> +    MemoryClassInfo<RenderStyle> info(memoryObjectInfo, this, MemoryInstrumentation::CSS);
> 
> Please do not submit additional instrumentation until we find a way to maintain it.

Done

[Pavel Feldman]

Comment on attachment 153479 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153479&action=review

> Source/WebCore/rendering/style/RenderStyle.cpp:1490
> +struct InstrumentedRenderStyle : public RefCounted<InstrumentedRenderStyle> {

I like the approach, but it puts additional burden to the developers. We need to agree on the approach in webkit-dev prior to proceeding.

[Alexei Filippov]

Created attachment 153820 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 153820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153820&action=review

> Source/WebCore/rendering/style/RenderStyle.cpp:1507
> +    COMPILE_ASSERT(sizeof(RenderStyle) == sizeof(InstrumentedRenderStyle), reportMemoryUsage_may_need_an_update);

Let's remove such checks as there were objections against the approach on webkit-dev. The validation tool that Ilya has been working on should give us enough confidence in the instrumentation code correctness.

[Alexei Filippov]

Comment on attachment 153820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153820&action=review

>> Source/WebCore/rendering/style/RenderStyle.cpp:1507
>> +    COMPILE_ASSERT(sizeof(RenderStyle) == sizeof(InstrumentedRenderStyle), reportMemoryUsage_may_need_an_update);
> 
> Let's remove such checks as there were objections against the approach on webkit-dev. The validation tool that Ilya has been working on should give us enough confidence in the instrumentation code correctness.

Ok. These checks are absent in the previous patch.

[Ilya Tikhonovsky]

Comment on attachment 153471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153471&action=review

> Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp:300
> +    info.addMember(m_flexibleBox.get());
> +    info.addMember(m_marquee.get());
> +    info.addMember(m_multiCol.get());
> +    info.addMember(m_transform.get());

please remove .get() everywhere
and create a specialization of addMember for OwnPtr and RefPtr

[Yury Semikhatsky]

Comment on attachment 153471 [details]
Patch

r- per Ilya's comment.

[Alexei Filippov]

Created attachment 154702 [details]
Patch

[Alexei Filippov]

Comment on attachment 153471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153471&action=review

>> Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp:300
>> +    info.addMember(m_transform.get());
> 
> please remove .get() everywhere
> and create a specialization of addMember for OwnPtr and RefPtr

Done

[Ilya Tikhonovsky]

Comment on attachment 154702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154702&action=review

> Source/WebCore/dom/MemoryInstrumentation.h:119
> +    template <typename T>
> +    struct OwningTraits<RefPtr<T> > {
> +        static void addInstrumentedMember(MemoryInstrumentation* instrumentation, const RefPtr<T>& t) { instrumentation->addInstrumentedMember(t.get()); }
> +        static void addMember(MemoryInstrumentation* instrumentation, const RefPtr<T>& t, MemoryInstrumentation::ObjectType objectType) { instrumentation->addMember(t.get(), objectType); }
> +    };
> +
>      template <typename T> void addInstrumentedMemberImpl(const T* const&, OwningType);
>      template <typename T> void addInstrumentedMemberImpl(const OwnPtr<T>* const& object, MemoryInstrumentation::OwningType owningType) { addInstrumentedMemberImpl(object->get(), owningType); }
>      template <typename T> void addInstrumentedMemberImpl(const RefPtr<T>* const& object, MemoryInstrumentation::OwningType owningType) { addInstrumentedMemberImpl(object->get(), owningType); }

It would be simpler to add 'template <typename T> void addMemberImpl(const RefPtr<T>* const& object,.....' as I've done for the instrumented members. It'd cost us only 3 loc. and one line for the instrumented DataRef<T>

> Source/WebCore/dom/Node.cpp:2804
> +struct InstrumentedNode : public EventTarget, public ScriptWrappable, public TreeShared<Node, ContainerNode> {
> +    uint32_t flags;
> +    void* ptrs[4];
> +};
> +

please remove this.

> Source/WebCore/dom/Node.cpp:2807
> +    COMPILE_ASSERT(sizeof(Node) == sizeof(InstrumentedNode), reportMemoryUsage_may_need_an_update);

ditto

[Build Bot]

Comment on attachment 154702 [details]
Patch

Attachment 154702 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13375020

[Build Bot]

Comment on attachment 154702 [details]
Patch

Attachment 154702 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13367757

[Alexei Filippov]

Created attachment 155258 [details]
Patch

[Alexei Filippov]

Comment on attachment 154702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154702&action=review

> Source/WebCore/dom/MemoryInstrumentation.h:119
>      template <typename T> void addInstrumentedMemberImpl(const RefPtr<T>* const& object, MemoryInstrumentation::OwningType owningType) { addInstrumentedMemberImpl(object->get(), owningType); }

done

>> Source/WebCore/dom/Node.cpp:2804
>> +
> 
> please remove this.

done

>> Source/WebCore/dom/Node.cpp:2807
>> +    COMPILE_ASSERT(sizeof(Node) == sizeof(InstrumentedNode), reportMemoryUsage_may_need_an_update);
> 
> ditto

done

[Build Bot]

Comment on attachment 155258 [details]
Patch

Attachment 155258 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13401026

[Build Bot]

Comment on attachment 155258 [details]
Patch

Attachment 155258 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13393310

[Alexei Filippov]

Created attachment 155270 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 155270 [details]
Patch

lgtm

[Alexei Filippov]

Created attachment 155282 [details]
Patch

[Alexei Filippov]

rebaselined

[Yury Semikhatsky]

Comment on attachment 155282 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155282&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Please provide a brief description of the patch.

> Source/WebCore/dom/MemoryInstrumentation.h:34
> +#include "DataRef.h"

This way we will include Source/WebCore/rendering/style/DataRef.h into other parts of WebCore. Can we avoid this?

> Source/WebCore/dom/MemoryInstrumentation.h:35
> +#include <stdio.h>

Revert this.

> Source/WebCore/rendering/style/RenderStyle.cpp:1504
> +        info.addVector(*m_cachedPseudoStyles);

This call will not add sizeof(Vector). One way to fix this would be adding of a specialization for Vector* I think it would make sense as I faced similar problem while instrumenting StylePropertySet.

[Alexei Filippov]

Created attachment 155523 [details]
Patch

[Alexei Filippov]

Comment on attachment 155282 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155282&action=review

>> Source/WebCore/ChangeLog:6
>> +        Reviewed by NOBODY (OOPS!).
> 
> Please provide a brief description of the patch.

done

>> Source/WebCore/dom/MemoryInstrumentation.h:34
>> +#include "DataRef.h"
> 
> This way we will include Source/WebCore/rendering/style/DataRef.h into other parts of WebCore. Can we avoid this?

done

>> Source/WebCore/dom/MemoryInstrumentation.h:35
>> +#include <stdio.h>
> 
> Revert this.

oops. done.

>> Source/WebCore/rendering/style/RenderStyle.cpp:1504
>> +        info.addVector(*m_cachedPseudoStyles);
> 
> This call will not add sizeof(Vector). One way to fix this would be adding of a specialization for Vector* I think it would make sense as I faced similar problem while instrumenting StylePropertySet.

fixed.

[Alexei Filippov]

Created attachment 155798 [details]
Patch

[Alexei Filippov]

rebaselined

[Yury Semikhatsky]

Comment on attachment 155798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155798&action=review

> Source/WebCore/dom/MemoryInstrumentation.h:132
> +    template <typename T> void addObjectImpl(const DataRef<T>* const& object, ObjectType objectType, OwningType) { addObjectImpl(object->get(), objectType, byPointer); }

How does this compile given that you only have a forward declaration of DataRef?

[Alexei Filippov]

Comment on attachment 155798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155798&action=review

>> Source/WebCore/dom/MemoryInstrumentation.h:132
>> +    template <typename T> void addObjectImpl(const DataRef<T>* const& object, ObjectType objectType, OwningType) { addObjectImpl(object->get(), objectType, byPointer); }
> 
> How does this compile given that you only have a forward declaration of DataRef?

At the place of template instantiation DataRef definition should be available.

[Yury Semikhatsky]

Comment on attachment 155798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155798&action=review

> Source/WebCore/dom/MemoryInstrumentation.h:286
> +    if (!vector || visited(vector))

We should keep the old check in addition to the new one: visited(vector->data()) as several vectors may share same buffer.

[Alexei Filippov]

Created attachment 155804 [details]
Patch

[Alexei Filippov]

Comment on attachment 155798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155798&action=review

>> Source/WebCore/dom/MemoryInstrumentation.h:286
>> +    if (!vector || visited(vector))
> 
> We should keep the old check in addition to the new one: visited(vector->data()) as several vectors may share same buffer.

ok

[Alexei Filippov]

Created attachment 155805 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 155805 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155805&action=review

> Source/WebCore/dom/MemoryInstrumentation.h:286
> +    if (!vector || visited(vector) || !vector->data() || visited(vector->data()))

I think we should return immediately  if (!vector || visited(vector))
but otherwise even if vector->data() is already visited it would make
sense to add size of Vector itself if it is referenced by a pointer.

[Alexei Filippov]

Created attachment 156030 [details]
Patch

[Alexei Filippov]

Comment on attachment 155805 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155805&action=review

+ rebaselining

>> Source/WebCore/dom/MemoryInstrumentation.h:286
>> +    if (!vector || visited(vector) || !vector->data() || visited(vector->data()))
> 
> I think we should return immediately  if (!vector || visited(vector))
> but otherwise even if vector->data() is already visited it would make
> sense to add size of Vector itself if it is referenced by a pointer.

removed this part from the patch.

[WebKit Review Bot]

Comment on attachment 156030 [details]
Patch

Clearing flags on attachment: 156030

Committed r124452: <http://trac.webkit.org/changeset/124452>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.