Bug 77700 – Make CSSValuePool share values globally.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andreas Kling]

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.

[Andreas Kling]

Created attachment 125270 [details]
Mostly a patch

Let's see what EWS thinks of this.

[WebKit Review Bot]

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

[Antti Koivisto]

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.

[Antti Koivisto]

It might make sense to do the wrapper behavior change separately from the value pool refactoring.

[Alexey Proskuryakov]

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.

[Andreas Kling]

(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.

[Antti Koivisto]

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.

[Andreas Kling]

(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.

[Andreas Kling]

Created attachment 127738 [details]
Wrapper part of the change, amended for V8.

[Andreas Kling]

(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>.

[Geoffrey Garen]

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?

[Andreas Kling]

(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.

[James Robinson]

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>

[James Robinson]

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

[Eric Seidel (no email)]

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.

[Andreas Kling]

Created attachment 136350 [details]
Globalization patch

[Antti Koivisto]

Comment on attachment 136350 [details]
Globalization patch

r=me

[Andreas Kling]

Committed r113647: <http://trac.webkit.org/changeset/113647>