Bug 124056 - Make RenderBlockRareData into a hash.
Summary: Make RenderBlockRareData into a hash.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-08 11:05 PST by Dave Hyatt
Modified: 2013-11-12 14:49 PST (History)
6 users (show)

See Also:


Attachments
Patch to move into map (not ready for review) (8.12 KB, patch)
2013-11-08 11:06 PST, Dave Hyatt
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (369.52 KB, application/zip)
2013-11-08 15:56 PST, Build Bot
no flags Details
Patch (24.70 KB, patch)
2013-11-11 10:24 PST, Dave Hyatt
sam: review+
Details | Formatted Diff | Diff
Patch (24.67 KB, patch)
2013-11-11 11:30 PST, Dave Hyatt
andersca: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (696.41 KB, application/zip)
2013-11-11 12:25 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (797.03 KB, application/zip)
2013-11-11 12:52 PST, Build Bot
no flags Details
Address the typo in RenderBlockFlow::setMustDiscardMarginBefore (24.73 KB, patch)
2013-11-12 09:03 PST, Dave Hyatt
andersca: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2013-11-08 11:05:45 PST
Make RenderBlockRareData into a hash to get those 4 bytes back.
Comment 1 Dave Hyatt 2013-11-08 11:06:58 PST
Created attachment 216413 [details]
Patch to move into map (not ready for review)
Comment 2 Build Bot 2013-11-08 11:51:45 PST
Comment on attachment 216413 [details]
Patch to move into map (not ready for review)

Attachment 216413 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/22738544
Comment 3 Build Bot 2013-11-08 15:56:28 PST
Comment on attachment 216413 [details]
Patch to move into map (not ready for review)

Attachment 216413 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22778562

New failing tests:
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-circle-001.html
fast/shapes/shape-inside/shape-inside-bottom-edge.html
fast/shapes/parsing/parsing-shape-lengths.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-000.html
fast/shapes/shape-inside/shape-inside-calc-crash.html
fast/regions/shape-inside/shape-inside-on-additional-regions.html
fast/shapes/shape-outside-floats/shape-outside-dynamic-shape-image-threshold.html
fast/shapes/shape-inside/floats/shape-inside-left-float-in-lower-left-triangle-block-content.html
http/tests/css/css-image-valued-shape.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-002.html
http/tests/security/shape-image-cors.html
fast/shapes/shape-inside/floats/shape-inside-left-float-in-lower-right-triangle-inline-content.html
fast/shapes/parsing/parsing-shape-outside.html
fast/regions/shape-inside/shape-inside-on-multiple-autoheight-regions.html
fast/regions/shape-inside/shape-inside-on-first-region-inline-content.html
fast/shapes/shape-inside/floats/shape-inside-left-float-in-lower-left-triangle-inline-content.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-004.html
fast/shapes/shape-outside-floats/shape-outside-animation.html
fast/shapes/shape-outside-floats/shape-outside-dynamic-shape-margin.html
fast/shapes/shape-inside/floats/shape-inside-left-float-in-lower-right-triangle-block-content.html
fast/shapes/parsing/parsing-shape-inside.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-005.html
fast/shapes/shape-inside/shape-inside-box-sizing.html
fast/shapes/shape-inside/shape-inside-animation.html
fast/regions/shape-inside/shape-inside-on-first-region-block-content.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-006.html
fast/shapes/css-shapes-enabled.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-003.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-circle-000.html
csswg/contributors/adobe/submitted/shapes/shape-outside/shape-outside-floats-clipped-001.html
Comment 4 Build Bot 2013-11-08 15:56:30 PST
Created attachment 216455 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 5 Dave Hyatt 2013-11-11 10:24:40 PST
Created attachment 216587 [details]
Patch
Comment 6 WebKit Commit Bot 2013-11-11 10:26:10 PST
Attachment 216587 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlock.h', u'Source/WebCore/rendering/RenderBlockFlow.cpp', u'Source/WebCore/rendering/RenderBlockFlow.h']" exit_code: 1
Source/WebCore/rendering/RenderBlockFlow.h:525:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Sam Weinig 2013-11-11 10:33:17 PST
Comment on attachment 216587 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:129
> +typedef WTF::HashMap<const RenderBlock*, OwnPtr<RenderBlockRareData>> RenderBlockRareDataMap;

I would remove the WTF:: from HashMap (its not needed) and make it a std::unique_ptr<RenderBlockRareData>.
Comment 8 Dave Hyatt 2013-11-11 11:30:15 PST
Created attachment 216591 [details]
Patch
Comment 9 Anders Carlsson 2013-11-11 11:32:37 PST
Comment on attachment 216591 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:225
> +    if (hasRareData())
> +        gRareDataMap->take(this);

Iā€™d just do an if (g_rareDataMap) g_rareDataMap->remove(this) here to avoid two hash lookups (one for the hasRareData and another for the actual remove).

> Source/WebCore/rendering/RenderBlock.cpp:1416
> +static RenderBlockRareData* ensureRareData(const RenderBlock* block)

I think this should return a reference since it can never be null.
Comment 10 Build Bot 2013-11-11 12:01:01 PST
Comment on attachment 216591 [details]
Patch

Attachment 216591 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/22908437
Comment 11 Build Bot 2013-11-11 12:25:33 PST
Comment on attachment 216591 [details]
Patch

Attachment 216591 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22868501

New failing tests:
fast/block/margin-collapse/webkit-margin-collapse-floats.html
Comment 12 Build Bot 2013-11-11 12:25:36 PST
Created attachment 216597 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 13 Build Bot 2013-11-11 12:52:08 PST
Comment on attachment 216591 [details]
Patch

Attachment 216591 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22539439

New failing tests:
js/basic-set.html
fast/block/margin-collapse/webkit-margin-collapse-floats.html
Comment 14 Build Bot 2013-11-11 12:52:11 PST
Created attachment 216602 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 15 Dave Hyatt 2013-11-12 09:03:56 PST
Created attachment 216681 [details]
Address the typo in RenderBlockFlow::setMustDiscardMarginBefore

Note I had made hasRareData() private in RenderBlock to avoid this mistake, but right now RenderBlockFlow is a friend of RenderBlock, so it was able to use hasRareData without it being a compile error. Annoying. Once we finish the refactoring, we'll want to make sure to remove the friend connection.
Comment 16 Build Bot 2013-11-12 09:47:02 PST
Comment on attachment 216681 [details]
Address the typo in RenderBlockFlow::setMustDiscardMarginBefore

Attachment 216681 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/22719636
Comment 17 Dave Hyatt 2013-11-12 14:49:14 PST
Landed in r159150.