Bug 116108 – Improve -webkit-text-underline-position memory usage

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Lamarque V. Souza]

-webkit-text-underline-position support was implemented in [1]. Eric Seidel has some concerns about memory usage [2] because of the private variable m_maxLogicalTop added to all RootInlineBox objects. This bug intents to improve memory usage of -webkit-text-underline-position by adding a cache for m_maxLogicalTop that will be used only when -webkit-text-underline-position is used in the page. That way if the page does not use -webkit-text-underline-position it should not hit memory overhead.

[1] http://trac.webkit.org/changeset/146104
[2] https://codereview.chromium.org/14576017#msg13

[Lamarque V. Souza]

Hi, opening a complex website creates about 2000 RootInlineBoxes, then the m_maxLogicalTop variable for all of them takes up about 16 KB (8 bytes per RootInlineBox). On the other hand if I just call computeMaxLogicalTop() everytime RootInlineBox::maxLogicalTop() is called it takes from 1 to 3 microseconds to run in my laptop. That looks pretty fast and I am not sure if implementing a cache is worthy now.

[Lamarque V. Souza]

Created attachment 201968 [details]
Patch

Proposed patch. Change RootInlineBox::maxLogicalTop() to compute the maxLogicalTop everytime it is called. The cpu power needed to compute maxLogicalTop is small and by removing m_maxLogicalTop variable we can reduce the memory used to store RootInlineBoxes.

[Early Warning System Bot]

Comment on attachment 201968 [details]
Patch

Attachment 201968 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/480494

[Early Warning System Bot]

Comment on attachment 201968 [details]
Patch

Attachment 201968 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/486092

[Build Bot]

Comment on attachment 201968 [details]
Patch

Attachment 201968 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/486095

[Lamarque V. Souza]

Created attachment 201972 [details]
Patch

Fix build with --css3-text disabled and remove obsolete comment.

[Benjamin Poulain]

Comment on attachment 201972 [details]
Patch

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

The patch looks like a good idea but I have concerns about constness.

Either those functions are badly named, or they should be const.

> Source/WebCore/rendering/RootInlineBox.cpp:345
> +float RootInlineBox::maxLogicalTop()
> +{
> +    float maxLogicalTop = 0;
> +    computeMaxLogicalTop(maxLogicalTop);
> +    return maxLogicalTop;
> +}
> +#endif // CSS3_TEXT

Looks like computeMaxLogicalTop() should be const. Then maxLogicalTop() should be const too.

[Lamarque V. Souza]

Created attachment 201998 [details]
Patch

Make computeMaxLogicalTop and maxLogicalTop const.

[Benjamin Poulain]

Comment on attachment 201998 [details]
Patch

LGTM.

[Simon Fraser (smfr)]

Comment on attachment 201998 [details]
Patch

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

We should add compile-time size assertions for RootInlineBox and friends so this doesn’t happen again.

> Source/WebCore/ChangeLog:11
> +        the maxLogicalTop value everytime it is called. In a tipical page the

typical

> Source/WebCore/ChangeLog:12
> +        number of interactions to calcule it is less than 10, so it is a small

‘interactions’ is an odd word to use here. Calls? Also, calculate.

[Lamarque V. Souza]

(In reply to comment #10)
> (From update of attachment 201998 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201998&action=review
> 
> We should add compile-time size assertions for RootInlineBox and friends so this doesn’t happen again.

I will submit a patch for that.
 
> > Source/WebCore/ChangeLog:12
> > +        number of interactions to calcule it is less than 10, so it is a small
> 
> ‘interactions’ is an odd word to use here. Calls? Also, calculate.

It should have been "iterations", I wrote that word because of the "for" loop in computeMaxLogicalTop, but "calls" looks better. What I want to explain is that computeMaxLogicalTop needs to iterate through all descendants of the RootInlineBox. The number of descendants is usually very small (less than 10) by what I have checked.

[Lamarque V. Souza]

Created attachment 202081 [details]
Patch

Update ChangeLog.

[WebKit Commit Bot]

Comment on attachment 202081 [details]
Patch

Rejecting attachment 202081 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--non-interactive', 202081, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/487419

[Bruno Abinader (history only)]

(In reply to comment #13)
> (From update of attachment 202081 [details])
> Rejecting attachment 202081 [details] from commit-queue.
> 
> Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--non-interactive', 202081, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
> 
> /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> 
> Full output: http://webkit-queues.appspot.com/results/487419

Oops :P Lamarque, can you please update the "Reviewed by" section from ChangeLog?

[Lamarque V. Souza]

Created attachment 202086 [details]
Patch

Update ChangeLog.

[Bruno Abinader (history only)]

Comment on attachment 202086 [details]
Patch

CQ'ing again :)

[WebKit Commit Bot]

Comment on attachment 202086 [details]
Patch

Clearing flags on attachment: 202086

Committed r150258: <http://trac.webkit.org/changeset/150258>