Teach the IDL compiler about TreatNullAs=EmptyString

Teach the IDL compiler about TreatNullAs=EmptyString

http://heycam.github.io/webidl/#TreatNullAs

We already have TreatNullAs=NullString, but this is not equivalent, as
the C++ code can differentiate between null and empty strings. The
extent to which this is actually done has not been surveyed, but it's
likely that some such checks can be removed in the process of
converting (non-reflected) attributes from TreatNullAs=NullString to
EmptyString.

BUG=334519

[philipj_slow, 2014-01-15 10:04:00]

https://codereview.chromium.org/139293004/diff/1/Source/bindings/v8/V8StringR...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/139293004/diff/1/Source/bindings/v8/V8StringR...
Source/bindings/v8/V8StringResource.h:169: TreatNullAsEmptyString,
The naming here isn't impressive, but I couldn't think of anything starting with
"With" that would make any sense. I could rename the two others for consistency
if so requested, or leave them alone in the hope that they will one day be
gone...

https://codereview.chromium.org/139293004/diff/1/Source/bindings/v8/V8StringR...
Source/bindings/v8/V8StringResource.h:240: if (m_v8Object.IsEmpty() ||
m_v8Object->IsNull()) {
I'm curious what the IsEmpty() check is and whether it's really needed. It's
used in the existing prepare()s, but when is the v8 wrapper ever empty? Removing
the check here would hit the check in prepareBase() instead, but would then
leave us with a null string instead of an empty string. That makes it extra
weird that the existing code has the check, since removing it should in both
cases have no observable effect...?

[haraken, 2014-01-15 11:57:36]

This CL looks OK, but I don't want to have both [TreatNullAs=NullString] and
[TreatNullAs=EmptyString]. It's confusing.

Would you mind trying to replace all [TreatNullAs=NullString] with
[TreatNullAs=EmptyString] and observe how perf bots look? If performance
permits, we want to go that way.

https://codereview.chromium.org/139293004/diff/60001/Source/bindings/v8/V8Str...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/139293004/diff/60001/Source/bindings/v8/V8Str...
Source/bindings/v8/V8StringResource.h:171: WithUndefinedOrNullCheck

Shall we also rename WithNullCheck to TreatNullAsNullString, and
WithUndefinedOrNullCheck to TreatUndefinedOrNullAsNullString ?

[haraken, 2014-01-15 12:00:06]

On 2014/01/15 11:57:36, haraken wrote:
> This CL looks OK, but I don't want to have both [TreatNullAs=NullString] and
> [TreatNullAs=EmptyString]. It's confusing.
> 
> Would you mind trying to replace all [TreatNullAs=NullString] with
> [TreatNullAs=EmptyString] and observe how perf bots look? If performance
> permits, we want to go that way.

Specifically:

(1) You can run Dromaeo locally. If performance regresses in Dromaeo, let's give
up the effort.
(2) Otherwise, let's land the replacement and observe perf bots. If we observe
any regression in perf bots, let's revert the CL.

WDYT?

[arv (Not doing code reviews), 2014-01-15 15:44:58]

https://codereview.chromium.org/139293004/diff/1/Source/bindings/v8/V8StringR...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/139293004/diff/1/Source/bindings/v8/V8StringR...
Source/bindings/v8/V8StringResource.h:240: if (m_v8Object.IsEmpty() ||
m_v8Object->IsNull()) {
On 2014/01/15 10:04:01, philipj wrote:
> I'm curious what the IsEmpty() check is and whether it's really needed. It's
> used in the existing prepare()s, but when is the v8 wrapper ever empty?
Removing
> the check here would hit the check in prepareBase() instead, but would then
> leave us with a null string instead of an empty string. That makes it extra
> weird that the existing code has the check, since removing it should in both
> cases have no observable effect...?

An empty handle could mean that the value is undefined or that there was a
problem (ie problem allocating the object, some exception etc).

Haraken, please verify the above.

https://codereview.chromium.org/139293004/diff/60001/Source/bindings/v8/V8Str...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/139293004/diff/60001/Source/bindings/v8/V8Str...
Source/bindings/v8/V8StringResource.h:171: WithUndefinedOrNullCheck
On 2014/01/15 11:57:36, haraken wrote:
> 
> Shall we also rename WithNullCheck to TreatNullAsNullString, and
> WithUndefinedOrNullCheck to TreatUndefinedOrNullAsNullString ?

sounds good to me

[philipj_slow, 2014-01-15 15:49:14]

https://codereview.chromium.org/139293004/diff/1/Source/bindings/v8/V8StringR...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/139293004/diff/1/Source/bindings/v8/V8StringR...
Source/bindings/v8/V8StringResource.h:240: if (m_v8Object.IsEmpty() ||
m_v8Object->IsNull()) {
On 2014/01/15 15:44:58, arv wrote:
> On 2014/01/15 10:04:01, philipj wrote:
> > I'm curious what the IsEmpty() check is and whether it's really needed. It's
> > used in the existing prepare()s, but when is the v8 wrapper ever empty?
> Removing
> > the check here would hit the check in prepareBase() instead, but would then
> > leave us with a null string instead of an empty string. That makes it extra
> > weird that the existing code has the check, since removing it should in both
> > cases have no observable effect...?
> 
> An empty handle could mean that the value is undefined or that there was a
> problem (ie problem allocating the object, some exception etc).
> 
> Haraken, please verify the above.

Just undefined in the JavaScript sense seems unlikely, since there's both
m_v8Object.IsEmpty() and m_v8Object->IsUndefined()

[philipj_slow, 2014-01-15 15:54:27]

On 2014/01/15 12:00:06, haraken wrote:
> On 2014/01/15 11:57:36, haraken wrote:
> > This CL looks OK, but I don't want to have both [TreatNullAs=NullString] and
> > [TreatNullAs=EmptyString]. It's confusing.
> > 
> > Would you mind trying to replace all [TreatNullAs=NullString] with
> > [TreatNullAs=EmptyString] and observe how perf bots look? If performance
> > permits, we want to go that way.
> 
> Specifically:
> 
> (1) You can run Dromaeo locally. If performance regresses in Dromaeo, let's
give
> up the effort.
> (2) Otherwise, let's land the replacement and observe perf bots. If we observe
> any regression in perf bots, let's revert the CL.
> 
> WDYT?

I'd also like to end up with only one of [TreatNullAs=NullString] and
[TreatNullAs=EmptyString] (hopefully the latter) so I agree that it's a good
idea to determine early whether or not performance is going to get in the way.

The Dromaeo test is big, so I think any regression is going to drown in the
rest. The "DOM Attributes" test group would be the relevant one, but I checked
the source and found that it's just setting the id property, which doesn't have
[TreatNullAs=NullString].

I'm going to try to write a target worst-case test to compare the current code
with one where V8StringResource<WithNullCheck>::prepare() has a one-line change
to use an empty string instead of a null string.

[arv (Not doing code reviews), 2014-01-15 15:59:00]

https://codereview.chromium.org/139293004/diff/1/Source/bindings/v8/V8StringR...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/139293004/diff/1/Source/bindings/v8/V8StringR...
Source/bindings/v8/V8StringResource.h:240: if (m_v8Object.IsEmpty() ||
m_v8Object->IsNull()) {
On 2014/01/15 15:49:14, philipj wrote:
> On 2014/01/15 15:44:58, arv wrote:
> > On 2014/01/15 10:04:01, philipj wrote:
> > > I'm curious what the IsEmpty() check is and whether it's really needed.
It's
> > > used in the existing prepare()s, but when is the v8 wrapper ever empty?
> > Removing
> > > the check here would hit the check in prepareBase() instead, but would
then
> > > leave us with a null string instead of an empty string. That makes it
extra
> > > weird that the existing code has the check, since removing it should in
both
> > > cases have no observable effect...?
> > 
> > An empty handle could mean that the value is undefined or that there was a
> > problem (ie problem allocating the object, some exception etc).
> > 
> > Haraken, please verify the above.
> 
> Just undefined in the JavaScript sense seems unlikely, since there's both
> m_v8Object.IsEmpty() and m_v8Object->IsUndefined()

Good point. I checked the source instead of making assumptions.

IsEmpty is a null pointer, in other words the handle is empty.

V8_INLINE bool IsEmpty() const { return val_ == 0; }

https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&l=244

[philipj_slow, 2014-01-15 16:39:49]

I wrote a benchmark to test only this aspect:

http://jsperf.com/setting-htmlbodyelement-alink

Here are the results in Ops/sec for "Set to null":

Using emptyString() in V8StringResource<WithNullCheck>::prepare(): 112,596±0.54%

Using String() in V8StringResource<WithNullCheck>::prepare(): 1,992,318±1.56%

What I wanted is something like 18 time slower, ouch! With this information, I
am going to abandon this CL and close the bug as WontFix.

(Ignore the "Set to red" test, I added it only to please jsperf.com. FWIW, its
numbers were pretty stable over the two runs.)

Should I do something with my perf test so that others in the future don't make
the change without seeing the performance regression?

[philipj_slow, 2014-01-15 16:46:26]

I should also mention that I was testing with a debug build, because building a
release build from scratch is slow on my machine. For completeness I'll try a
release build tomorrow...

[philipj_slow, 2014-01-16 02:32:56]

The numbers for release are 25,316,019±0.40% vs 1,357,553±0.45%, so my way is
still 18 times slower :/

Case closed.

[haraken, 2014-01-16 02:37:16]

On 2014/01/16 02:32:56, philipj wrote:
> The numbers for release are 25,316,019±0.40% vs 1,357,553±0.45%, so my way is
> still 18 times slower :/
> 
> Case closed.

Thanks for the investigation, philipj! The result is really helpful to
understand our current code.

Just to clarify: Do you need [TreatNullAs=EmptyString] to fix some web-exposed
behavior?

[philipj_slow, 2014-01-16 02:58:09]

On 2014/01/16 02:37:16, haraken wrote:
> On 2014/01/16 02:32:56, philipj wrote:
> > The numbers for release are 25,316,019±0.40% vs 1,357,553±0.45%, so my way
is
> > still 18 times slower :/
> > 
> > Case closed.
> 
> Thanks for the investigation, philipj! The result is really helpful to
> understand our current code.
> 
> Just to clarify: Do you need [TreatNullAs=EmptyString] to fix some web-exposed
> behavior?

Not that I know of, but what originally prompted me to do this is the isNull()
check in HTMLSelectElement::setValue(). value used to have
[TreatNullAs=NullString] and when it did setting value to null and "" would not
have followed the same code path, and indeed have different Web-exposed
behavior. Fiddle with this in a Chromium older than Blink r163087 to verify:

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/2750

The reason I didn't remove the null check together with [TreatNullAs=NullString]
is that the code is also reachable from WebSelectElement, and I couldn't
determine whether it could ever send in null in the twists and turns of form
autofill :/

My concern is that we might have more cases like this with subtly different
behavior for "" and null. It's not an issue for reflected attributes, so
hopefully after my war on [TreatNullAs=NullString] is reaching the end it'll be
possible to manually check the remaining cases and write test cases to verify.