Summary: | Improve -webkit-text-underline-position memory usage | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Lamarque V. Souza <lamarque> | ||||||||||||
Component: | CSS | Assignee: | 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
Lamarque V. Souza
2013-05-14 09:29:40 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. 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 on attachment 201968 [details] Patch Attachment 201968 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/480494 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 on attachment 201968 [details] Patch Attachment 201968 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/486095 Created attachment 201972 [details]
Patch
Fix build with --css3-text disabled and remove obsolete comment.
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. Created attachment 201998 [details]
Patch
Make computeMaxLogicalTop and maxLogicalTop const.
Comment on attachment 201998 [details]
Patch
LGTM.
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. (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. Created attachment 202081 [details]
Patch
Update ChangeLog.
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 (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? Created attachment 202086 [details]
Patch
Update ChangeLog.
Comment on attachment 202086 [details]
Patch
CQ'ing again :)
Comment on attachment 202086 [details] Patch Clearing flags on attachment: 202086 Committed r150258: <http://trac.webkit.org/changeset/150258> |