New issue
Advanced search Search tips

Issue 724565 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 727804

Blocking:
issue 722511
issue 730692



Sign in to add a comment

Make CSS Parser Worker friendly

Project Member Reported by fs...@chromium.org, May 19 2017

Issue description

We identified a subset of the CSS parser that must always work on Workers (filters, colors, dimensions, font). For this to be true, code needs to be thread-safe and Document independent.

- clear CSS parser/resolver of static code. Either DCHECK thread or make the code fully thread safe.
- add tests that exercise the parser on a separate thread
- add tests that exercise CSS parser/resolver without a document

 
Labels: Update-Quarterly
Labels: -Update-Quarterly Update-Monthly
Project Member

Comment 3 by bugdroid1@chromium.org, May 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/060a08af91bdc5a090a78e852d4470c52458c004

commit 060a08af91bdc5a090a78e852d4470c52458c004
Author: fserb <fserb@chromium.org>
Date: Sat May 27 03:03:56 2017

Test util to allow MultiThreaded tests on TSan

BUG= 724565 

Review-Url: https://codereview.chromium.org/2912523002
Cr-Commit-Position: refs/heads/master@{#475227}

[add] https://crrev.com/060a08af91bdc5a090a78e852d4470c52458c004/third_party/WebKit/Source/core/css/threaded/MultiThreadedTestUtil.h

Project Member

Comment 4 by bugdroid1@chromium.org, May 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/335f92a2a16c9a041c693938b9faa66516f0713e

commit 335f92a2a16c9a041c693938b9faa66516f0713e
Author: Fernando Serboncini <fserb@google.com>
Date: Mon May 29 21:46:34 2017

Makes MultiThreadedTestUtil useful

- Adds new TSAN_TEST and TSAN_TEST_F macros for threaded tests
- Replace WebThread with WebThreadSupportingGC

Bug:  724565 
Change-Id: Ic8a64d268049c56f5e70611e28ecad9ab07e7ec9
Reviewed-on: https://chromium-review.googlesource.com/518262
Reviewed-by: Justin Novosad <junov@chromium.org>
Commit-Queue: Fernando Serboncini <fserb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#475385}
[modify] https://crrev.com/335f92a2a16c9a041c693938b9faa66516f0713e/third_party/WebKit/Source/core/css/threaded/MultiThreadedTestUtil.h

Comment 5 by fs...@chromium.org, May 30 2017

Blockedon: 727804
Project Member

Comment 6 by bugdroid1@chromium.org, May 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78e08652d53f5b2c221074203c7c1d986afd4ec4

commit 78e08652d53f5b2c221074203c7c1d986afd4ec4
Author: Fernando Serboncini <fserb@google.com>
Date: Wed May 31 19:05:26 2017

Allow CSSToLengthConversionData to work without a ComputedStyle

Bug:  724565 
Change-Id: I3580c848d5e13a0be3e6364f977ac3d807ef4127
Reviewed-on: https://chromium-review.googlesource.com/518926
Commit-Queue: Fernando Serboncini <fserb@chromium.org>
Reviewed-by: Justin Novosad <junov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#475968}
[modify] https://crrev.com/78e08652d53f5b2c221074203c7c1d986afd4ec4/third_party/WebKit/Source/core/css/CSSToLengthConversionData.cpp
[modify] https://crrev.com/78e08652d53f5b2c221074203c7c1d986afd4ec4/third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4f75e35ab706cc59d28172071a44df09a5129af9

commit 4f75e35ab706cc59d28172071a44df09a5129af9
Author: Fernando Serboncini <fserb@chromium.org>
Date: Thu Jun 01 21:56:12 2017

Make CSSParser multi threaded, adds OffscreenCanvas filters thread tests

- CSS Parser now is ThreadSpecific and not only static local.
- Multi threaded tests for all code that OffscreenCanvas filters
currently use:
  - CSSToLengthConversionData
  - CreateOffscreenFilterOperations

Bug:  724565 
Change-Id: I7fdc7f2ff0246709211740d8f708f65513aa2a4f
Reviewed-on: https://chromium-review.googlesource.com/517969
Commit-Queue: Fernando Serboncini <fserb@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Justin Novosad <junov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#476446}
[modify] https://crrev.com/4f75e35ab706cc59d28172071a44df09a5129af9/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/4f75e35ab706cc59d28172071a44df09a5129af9/third_party/WebKit/Source/core/css/CSSToLengthConversionData.h
[modify] https://crrev.com/4f75e35ab706cc59d28172071a44df09a5129af9/third_party/WebKit/Source/core/css/parser/CSSParserContext.cpp
[add] https://crrev.com/4f75e35ab706cc59d28172071a44df09a5129af9/third_party/WebKit/Source/core/css/threaded/CSSToLengthConversionDataThreadedTest.cpp
[add] https://crrev.com/4f75e35ab706cc59d28172071a44df09a5129af9/third_party/WebKit/Source/core/css/threaded/FilterOperationResolverThreadedTest.cpp
[modify] https://crrev.com/4f75e35ab706cc59d28172071a44df09a5129af9/third_party/WebKit/Source/core/css/threaded/MultiThreadedTestUtil.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2e48d743c9c647a66409c98ba96cbe80fd70774b

commit 2e48d743c9c647a66409c98ba96cbe80fd70774b
Author: Fernando Serboncini <fserb@chromium.org>
Date: Tue Jun 06 19:49:57 2017

Remove CSSTextCache from CSSPrimitiveValue

CSSTextCache isn't ever cleared. Its values are stored keyed by
elements not by value, so no reuse is possible. It really works as a
very leaky way of caching CustomCSSText on the same object.

There's no real world use case of requesting the same CSS value
multiple times, while not caching gives us a small performance gain
(due to not having to cache it) on single requests - which are 
overwhelmingly more common -, plus we get a nice memory gain of
not having those dangling strings forever there.

Local performance test of dromaeo.jslibstylejquery (that gets/sets 
CSS values on DOM objects) were on average 3% faster (most within 1
stddev). The best tests was 15% faster (4 stddev), the 2nd best 8%
faster (2 stddev). The worst test was 0.5 stddev below baseline.

Bug:  724565 
Change-Id: I5c13566763d36bb40d37808ae38355b937baec9d
Reviewed-on: https://chromium-review.googlesource.com/522107
Commit-Queue: Fernando Serboncini <fserb@chromium.org>
Reviewed-by: Justin Novosad <junov@chromium.org>
Reviewed-by: Eddy Mead <meade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477380}
[modify] https://crrev.com/2e48d743c9c647a66409c98ba96cbe80fd70774b/third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp
[modify] https://crrev.com/2e48d743c9c647a66409c98ba96cbe80fd70774b/third_party/WebKit/Source/core/css/CSSValue.h

Project Member

Comment 9 by bugdroid1@chromium.org, Jun 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24d73b789a35a325da5ec6690ee284a2a4a8b6b0

commit 24d73b789a35a325da5ec6690ee284a2a4a8b6b0
Author: Fernando Serboncini <fserb@chromium.org>
Date: Tue Jun 06 21:21:57 2017

Removes CSSIdentifierValue double caching of Strings

CSS identifier are already static char* mapped.

The removed function would cache an (thread racy) array - of
numCSSIdentifier size - of AtomicStrings (which are, also, cached on
AtomStringTable).

This gives some memory back (the 1k~ table), makes it thread friendly
and is performance neutral (we do one less indirection).

This was originally implemented on the transition to CSSValueID, but no
proper performance information was given.

Bug:  724565 
Change-Id: I6a900dc184433dbe737caa69a0e5b0788335e554
Reviewed-on: https://chromium-review.googlesource.com/526293
Commit-Queue: Fernando Serboncini <fserb@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477405}
[modify] https://crrev.com/24d73b789a35a325da5ec6690ee284a2a4a8b6b0/third_party/WebKit/Source/core/css/CSSIdentifierValue.cpp

Blocking: 730692
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/acdbf10d6795e77bab00ffbf2986037397ab96b0

commit acdbf10d6795e77bab00ffbf2986037397ab96b0
Author: Fernando Serboncini <fserb@chromium.org>
Date: Wed Jun 07 18:04:21 2017

Adds new CSSParser threaded tests + fixes data races related to Parsing

Add CSSParser tests for Filter and Font.
Uncovered and solved the following data races:

  - StylePropertyShorthandCustom (constexpr)
  - StylePropertyShorthand template (constexpr)
  - CSSPropertyMetadata template (make initialization atomic)

Bug:  724565 
Change-Id: I9a8d1acd9721a41d02d6d2447fa3f27935a761a1
Reviewed-on: https://chromium-review.googlesource.com/526473
Commit-Queue: Fernando Serboncini <fserb@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Justin Novosad <junov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477702}
[modify] https://crrev.com/acdbf10d6795e77bab00ffbf2986037397ab96b0/third_party/WebKit/Source/build/scripts/templates/CSSPropertyMetadata.cpp.tmpl
[modify] https://crrev.com/acdbf10d6795e77bab00ffbf2986037397ab96b0/third_party/WebKit/Source/build/scripts/templates/StylePropertyShorthand.cpp.tmpl
[modify] https://crrev.com/acdbf10d6795e77bab00ffbf2986037397ab96b0/third_party/WebKit/Source/build/scripts/templates/StylePropertyShorthand.h.tmpl
[modify] https://crrev.com/acdbf10d6795e77bab00ffbf2986037397ab96b0/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/acdbf10d6795e77bab00ffbf2986037397ab96b0/third_party/WebKit/Source/core/css/StylePropertyShorthandCustom.cpp
[add] https://crrev.com/acdbf10d6795e77bab00ffbf2986037397ab96b0/third_party/WebKit/Source/core/css/threaded/CSSParserThreadedTest.cpp
[modify] https://crrev.com/acdbf10d6795e77bab00ffbf2986037397ab96b0/third_party/WebKit/Source/core/css/threaded/FilterOperationResolverThreadedTest.cpp

Status: Fixed (was: Available)
The bulk of the work here is done.
Moving to platform/font.

Sign in to add a comment