Bug 77700 - Make CSSValuePool share values globally.
Summary: Make CSSValuePool share values globally.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks: 77745
  Show dependency treegraph
 
Reported: 2012-02-02 18:23 PST by Andreas Kling
Modified: 2012-04-09 17:47 PDT (History)
11 users (show)

See Also:


Attachments
Mostly a patch (225.96 KB, patch)
2012-02-02 23:19 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Wrapper part of the change, amended for V8. (16.51 KB, patch)
2012-02-19 11:03 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Globalization patch (240.81 KB, patch)
2012-04-09 17:30 PDT, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2012-02-02 18:23:08 PST
We currently limit CSSValuePool to sharing CSSValues inside the same Document. This is because the JSC wrappers are cached, and can have custom properties that we don't want multiple documents to have access to at the same time.

Because the CSSValue DOM interface is very rarely used, and is poorly supported by other engines, I think we should let CSSValue wrappers be unique objects (i.e calling CSSStyleDeclaration.getPropertyCSSValue('foo') twice would return two discrete objects.)

This will allow us to make CSSValuePool global, which will reduce memory usage and bring us one step closer to cacheable stylesheets.
Comment 1 Andreas Kling 2012-02-02 23:19:20 PST
Created attachment 125270 [details]
Mostly a patch

Let's see what EWS thinks of this.
Comment 2 WebKit Review Bot 2012-02-03 00:27:12 PST
Comment on attachment 125270 [details]
Mostly a patch

Attachment 125270 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11420198

New failing tests:
http/tests/security/cross-origin-css-primitive.html
fast/css/css-value-wrapper-sameness.html
Comment 3 Antti Koivisto 2012-02-03 00:41:07 PST
Comment on attachment 125270 [details]
Mostly a patch

Makes sense to me. While this is an observable behavior change, the case is so obscure it seems unlikely to matter in practice. The current behavior with wrapper identities is already rather surprising and that hasn't caused problems.
Comment 4 Antti Koivisto 2012-02-03 05:39:29 PST
It might make sense to do the wrapper behavior change separately from the value pool refactoring.
Comment 5 Alexey Proskuryakov 2012-02-03 11:37:49 PST
Do we have a way to check for performance impact? In scripts the repeatedly access the same value, this would cause additional garbage to collect.
Comment 6 Andreas Kling 2012-02-03 12:12:41 PST
(In reply to comment #5)
> Do we have a way to check for performance impact? In scripts the repeatedly access the same value, this would cause additional garbage to collect.

You are right of course, but I don't know of anyone or anything using the CSSValue interface to a greater extent in production. There is a single call to getPropertyCSSValue() in our Web Inspector JS code, so that's not helpful either.
Comment 7 Antti Koivisto 2012-02-03 13:02:52 PST
I think the only way to find out is to try it.

Computed styles (which are probably most of the real world use due to wider engine support) don't consume significant amounts of memory so if needed we could use unique (non-pool) CSSValues there and allow wrapper caching for those.
Comment 8 Andreas Kling 2012-02-03 13:13:27 PST
(In reply to comment #7)
> I think the only way to find out is to try it.
> 
> Computed styles (which are probably most of the real world use due to wider engine support) don't consume significant amounts of memory so if needed we could use unique (non-pool) CSSValues there and allow wrapper caching for those.

Indeed. Like we discussed on IRC, I'm tempted to move towards supporting the CSSValue interface only for computed styles. This would match Gecko, and allow us to simplify non-computed styles further. Completely outside the scope of this bug obviously.
Comment 9 Andreas Kling 2012-02-19 11:03:06 PST
Created attachment 127738 [details]
Wrapper part of the change, amended for V8.
Comment 10 Andreas Kling 2012-02-19 12:18:32 PST
(In reply to comment #9)
> Created an attachment (id=127738) [details]
> Wrapper part of the change, amended for V8.

This piece landed in <http://trac.webkit.org/changeset/108195>.
Comment 11 Geoffrey Garen 2012-02-19 14:02:41 PST
I believe this change is incompatible with WebIDL. Do you think there's a specification that comments on what's correct behavior here?

> Because the CSSValue DOM interface is very rarely used and is poorly supported by other engines

I see some unclear thinking here. These arguments cut equally against making performance improvements and preserving backwards-compatibility. If nobody uses or implements this API, neither behavior nor performance matter.

> This will allow us to make CSSValuePool global

It seems like you're trading more wrapper allocation for less underlying value allocation. Why is this trade a net win?
Comment 12 Andreas Kling 2012-02-19 14:52:53 PST
(In reply to comment #11)
> I believe this change is incompatible with WebIDL. Do you think there's a specification that comments on what's correct behavior here?

Not that I know of. If WebIDL does specify this behavior, I suspect we have a number of methods violating it by always returning a unique underlying value resulting in a unique wrapper.

> > Because the CSSValue DOM interface is very rarely used and is poorly supported by other engines
> 
> I see some unclear thinking here. These arguments cut equally against making performance improvements and preserving backwards-compatibility. If nobody uses or implements this API, neither behavior nor performance matter.

The "poorly supported" part refers to the fact that we are the only engine that implements getPropertyCSSValue() for non-computed style declarations. Furthermore, in the new CSSOM draft currently in development, CSSValue is basically stripped down to { .cssText }

Most importantly, this is by no means the intended end state. The per-document value caching was blocking Antti's ongoing CSS refactoring so he asked me to finish and land it. 

Over the next couple of weeks, we'll see the introduction of a new internal CSS value class (tentatively named StyleValue.) CSSValue wrapper creation will be managed by their logical owners, i.e the CSSStyleDeclaration they are extracted from. This will change our behavior slightly once again.

> > This will allow us to make CSSValuePool global
> 
> It seems like you're trading more wrapper allocation for less underlying value allocation. Why is this trade a net win?

Because the vast majority of pages never instantiate a single CSSValue wrapper, but hundreds if not thousands of CSSValue objects.
Comment 13 James Robinson 2012-02-19 15:33:16 PST
Reverted r108195 for reason:

Lots of failing ASSERT()s on v8 bots, requested by kling on #webkit

Committed r108200: <http://trac.webkit.org/changeset/108200>
Comment 14 James Robinson 2012-02-19 15:35:45 PST
I couldn't get a full list of layout tests that failed ASSERT()s in v8 debug since the bots early-exited after 20 crashes, but here's an example ASSERT() failure from a chromium linux debug bot on test fast/css/CSSPrimitiveValue-exceptions.html:

ASSERTION FAILED: !m_map.contains(obj)
third_party/WebKit/Source/WebCore/bindings/v8/V8DOMMap.h(91) : void WebCore::WeakReferenceMap<KeyType, ValueType>::set(KeyType*, v8::Persistent<ValueType>) [with KeyType = void, ValueType = v8::Object]
1   0x1500864
2   0x1a315b5
3   0x19067c2
4   0x1906800
5   0x253747e
6   0x1878c0f
7   0x198d3bc
8   0x8c476f
9   0x8bf206
10  0x8bf1d7
11  0x1ca037e0424e
[41817:41817:15198725650916:ERROR:process_util_posix.cc(142)] Received signal 11
	base::debug::StackTrace::StackTrace() [0x73193e]
	base::(anonymous namespace)::StackDumpSignalHandler() [0x6eb4d9]
	0x7f8f06ba5af0
	WebCore::WeakReferenceMap<>::set() [0x150086e]
	WebCore::V8CSSPrimitiveValue::wrapSlow() [0x1a315b5]
	WebCore::V8CSSPrimitiveValue::wrap() [0x19067c2]
	WebCore::toV8() [0x1906800]
	WebCore::toV8() [0x253747e]
	WebCore::toV8() [0x1878c0f]
	WebCore::CSSStyleDeclarationInternal::getPropertyCSSValueCallback() [0x198d3bc]
	v8::internal::HandleApiCallHelper<>() [0x8c476f]
	v8::internal::Builtin_Impl_HandleApiCall() [0x8bf206]
	v8::internal::Builtin_HandleApiCall() [0x8bf1d7]
	0x1ca037e0424e
None
Comment 15 Eric Seidel (no email) 2012-03-27 12:46:48 PDT
Comment on attachment 125270 [details]
Mostly a patch

Cleared Antti Koivisto's review+ from obsolete attachment 125270 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 16 Andreas Kling 2012-04-09 17:30:42 PDT
Created attachment 136350 [details]
Globalization patch
Comment 17 Antti Koivisto 2012-04-09 17:33:39 PDT
Comment on attachment 136350 [details]
Globalization patch

r=me
Comment 18 Andreas Kling 2012-04-09 17:47:37 PDT
Committed r113647: <http://trac.webkit.org/changeset/113647>