Bug 132332 - [CSS Grid Layout] Grid optimization keeping track of its status
Summary: [CSS Grid Layout] Grid optimization keeping track of its status
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks: 125145
  Show dependency treegraph
 
Reported: 2014-04-29 05:06 PDT by Manuel Rego Casasnovas
Modified: 2014-08-01 07:58 PDT (History)
13 users (show)

See Also:


Attachments
Patch (21.41 KB, patch)
2014-04-29 05:08 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (21.49 KB, patch)
2014-05-26 06:53 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (21.46 KB, patch)
2014-05-26 13:46 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (21.47 KB, patch)
2014-06-03 04:18 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (21.34 KB, patch)
2014-06-03 13:06 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (25.89 KB, patch)
2014-06-18 02:04 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2014-04-29 05:06:03 PDT
The following patch is a port of Blink https://chromiumcodereview.appspot.com/21205004

It's making grid layout perftest around 29% faster.
Comment 1 Manuel Rego Casasnovas 2014-04-29 05:08:49 PDT
Created attachment 230366 [details]
Patch
Comment 2 Manuel Rego Casasnovas 2014-05-26 06:53:12 PDT
Created attachment 232075 [details]
Patch

Rebased patch.
Comment 3 Manuel Rego Casasnovas 2014-05-26 13:46:38 PDT
Created attachment 232095 [details]
Patch

Fix build.
Comment 4 Manuel Rego Casasnovas 2014-06-03 04:18:20 PDT
Created attachment 232421 [details]
Patch

Upload rebased patch after r169385.
Comment 5 Radu Stavila 2014-06-03 06:38:10 PDT
Comment on attachment 232421 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:1201
> +    // We could avoid to dirty the grid if the new child has definite positions and grid auto-flow property is none.

The original patch from blink contained some additional logic here, it didn't always just dirty the grid. Is there a reason you removed that?

> Source/WebCore/rendering/RenderGrid.cpp:1219
> +    return nextSibling;

I think it would be clearer and better for future modifications to have a single return point. E.g.:

RenderObject* nextSibling = RenderBlock::removeChild(child);
if (!girdIsDirty())
   dirtyGrid();

return nextSibling;
Comment 6 Manuel Rego Casasnovas 2014-06-03 13:05:31 PDT
Comment on attachment 232421 [details]
Patch

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

I'm uploading a new patch changing RenderGrid::removeChild() as suggested.

>> Source/WebCore/rendering/RenderGrid.cpp:1201
>> +    // We could avoid to dirty the grid if the new child has definite positions and grid auto-flow property is none.
> 
> The original patch from blink contained some additional logic here, it didn't always just dirty the grid. Is there a reason you removed that?

Basically the patch in Blink was not right, as it was introducing some bugs some of them due to the extra logic added here.

I've plans to avoid marking the grid as dirty always, but that will be done in a later step.

>> Source/WebCore/rendering/RenderGrid.cpp:1219
>> +    return nextSibling;
> 
> I think it would be clearer and better for future modifications to have a single return point. E.g.:
> 
> RenderObject* nextSibling = RenderBlock::removeChild(child);
> if (!girdIsDirty())
>    dirtyGrid();
> 
> return nextSibling;

Nice suggestion, I've modified it.
Comment 7 Manuel Rego Casasnovas 2014-06-03 13:06:35 PDT
Created attachment 232436 [details]
Patch
Comment 8 Radu Stavila 2014-06-03 15:05:40 PDT
Comment on attachment 232421 [details]
Patch

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

>>> Source/WebCore/rendering/RenderGrid.cpp:1201
>>> +    // We could avoid to dirty the grid if the new child has definite positions and grid auto-flow property is none.
>> 
>> The original patch from blink contained some additional logic here, it didn't always just dirty the grid. Is there a reason you removed that?
> 
> Basically the patch in Blink was not right, as it was introducing some bugs some of them due to the extra logic added here.
> 
> I've plans to avoid marking the grid as dirty always, but that will be done in a later step.

Ok, sounds good.
Comment 9 Manuel Rego Casasnovas 2014-06-18 02:04:50 PDT
Created attachment 233300 [details]
Patch

New version of the patch including a new change from Blink (https://codereview.chromium.org/302083005).
Comment 10 Manuel Rego Casasnovas 2014-08-01 04:23:35 PDT
Comment on attachment 233300 [details]
Patch

This is causing some security issues in Blink, so until it gets stabilized we won't integrate it on WebKit. Probably a new rebase would be needed but that time.