Bug 116108

Summary: Improve -webkit-text-underline-position memory usage
Product: WebKit Reporter: Lamarque V. Souza <lamarque>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bruno.abinader, cbiesinger, commit-queue, esprehn+autocc, glenn, hyatt, kling, koivisto, simon.fraser, webkit-ews
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
benjamin: review+
Patch
commit-queue: commit-queue-
Patch none

Description Lamarque V. Souza 2013-05-14 09:29:40 PDT
-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
Comment 1 Lamarque V. Souza 2013-05-15 15:01:06 PDT
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.
Comment 2 Lamarque V. Souza 2013-05-16 10:21:10 PDT
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.
Comment 3 Early Warning System Bot 2013-05-16 10:26:43 PDT
Comment on attachment 201968 [details]
Patch

Attachment 201968 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/480494
Comment 4 Early Warning System Bot 2013-05-16 10:28:32 PDT
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
Comment 5 Build Bot 2013-05-16 10:48:05 PDT
Comment on attachment 201968 [details]
Patch

Attachment 201968 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/486095
Comment 6 Lamarque V. Souza 2013-05-16 10:58:51 PDT
Created attachment 201972 [details]
Patch

Fix build with --css3-text disabled and remove obsolete comment.
Comment 7 Benjamin Poulain 2013-05-16 15:27:37 PDT
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.
Comment 8 Lamarque V. Souza 2013-05-16 16:11:00 PDT
Created attachment 201998 [details]
Patch

Make computeMaxLogicalTop and maxLogicalTop const.
Comment 9 Benjamin Poulain 2013-05-16 16:25:05 PDT
Comment on attachment 201998 [details]
Patch

LGTM.
Comment 10 Simon Fraser (smfr) 2013-05-16 20:05:34 PDT
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.
Comment 11 Lamarque V. Souza 2013-05-17 06:45:29 PDT
(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.
Comment 12 Lamarque V. Souza 2013-05-17 07:14:23 PDT
Created attachment 202081 [details]
Patch

Update ChangeLog.
Comment 13 WebKit Commit Bot 2013-05-17 07:18:22 PDT
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
Comment 14 Bruno Abinader (history only) 2013-05-17 07:19:51 PDT
(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?
Comment 15 Lamarque V. Souza 2013-05-17 07:42:32 PDT
Created attachment 202086 [details]
Patch

Update ChangeLog.
Comment 16 Bruno Abinader (history only) 2013-05-17 07:43:33 PDT
Comment on attachment 202086 [details]
Patch

CQ'ing again :)
Comment 17 WebKit Commit Bot 2013-05-17 08:10:39 PDT
Comment on attachment 202086 [details]
Patch

Clearing flags on attachment: 202086

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