Replace OwnPtr with WTF::Optional for optional recorders.

Replace OwnPtr with WTF::Optional for optional recorders.

Also implement WTF::Optional, using an aligned buffer.

BUG=492743
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196267

[jbroman, 2015-05-08 15:13:27]

jbroman@chromium.org changed reviewers:
+ thakis@chromium.org

[jbroman, 2015-05-08 15:13:28]

It's a little silly to do heap allocation for this, so let's not.

Ideally I'd use std::optional, but that isn't standardized yet. Ideally I'd use
unrestricted unions to implement this, but MSVC doesn't support that yet. So
I've implemented the features of optional that I need for this, using an aligned
buffer. (Others could add assignment, re-emplacement, copying, swapping,
comparison, and other things, if desired in the future.)

Here are the changes I observed to record time on a Nexus 4 running
rasterize_and_record_micro against key_mobile_sites (I believe the rasterization
time change to be pure noise, since this code does not change rasterization at
all):
https://jeremyroman.github.io/crpub/1129793005-results.html

A more targeted microbenchmark that does a small amount of work with an optional
using either OwnPtr<> or Optional<> reveals (repeated 1M times) shows 400 ns per
iteration for OwnPtr (or 200 ns if the class uses WTF_MAKE_FAST_ALLOCATED) and
13 ns per iteration for Optional, due to the benefit of skipping the (expensive
on Android) allocation.

[chrishtr, 2015-05-18 23:17:47]

chrishtr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2015-05-18 23:17:48]

Ping on review.

Jeremy if you'd like I can also review this.

My inclination is that a simpler approach of implementing a split between
construction and init of the various recorder classes is a simple enough change
that I would suggest we go straight for that, unless you can find more examples
of conditional allocation patterns in Blink to justify the new Optional
interface.

[Nico, 2015-05-18 23:38:15]

This is "[chromium-dev] Should we add something like value_ptr<> to base/?" / 
https://code.google.com/p/chromium/issues/detail?id=428934 , right?

[esprehn, 2015-05-19 02:27:37]

I'd much rather we went with a simpler on stack scope object, and that scope
object contains an OwnPtr. That sounds similar to splitting the init/creation
that chrishtr is talking about.

I don't think we want this in WTF. It doesn't seem to add to readability with
the varargs construction.

[jbroman, 2015-05-21 17:05:57]

On 2015/05/18 23:17:48, chrishtr wrote:
> Ping on review.
> 
> Jeremy if you'd like I can also review this.
> 
> My inclination is that a simpler approach of implementing a split between
> construction and init of the various recorder classes is a simple enough
change
> that I would suggest we go straight for that, unless you can find more
examples
> of conditional allocation patterns in Blink to justify the new Optional
> interface.

It's a little hard to search for this sort of thing. Some occurrences of this
OwnPtr-for-optional-on-stack pattern that I have managed to find:
- contextScope in V8Debugger::setScriptSource
- both uses of OwnPtr<UserGestureIndicator> in EventHandler
- the use of OwnPtr<UserGestureIndicator> in
WebFrameWidgetImpl::handleInputEvent


On 2015/05/18 23:38:15, Nico (afk until Thu May 21) wrote:
> This is "[chromium-dev] Should we add something like value_ptr<> to base/?" / 
> https://code.google.com/p/chromium/issues/detail?id=428934 , right?

Yes, essentially. Note that the storage cost is less of a concern when the
objects
in question are short-lived stack objects, like these.


On 2015/05/19 02:27:37, esprehn wrote:
> I'd much rather we went with a simpler on stack scope object, and that scope
> object contains an OwnPtr. That sounds similar to splitting the init/creation
> that chrishtr is talking about.

That is equivalent to not applying this patch. The objective is to stop doing
(expensive) heap allocation simply because we don't know whether we'll need to
push an end display item or not.

> I don't think we want this in WTF. It doesn't seem to add to readability with
> the varargs construction.

I disagree.

Do we also intend to avoid the varargs-forwarding pattern when we finally get
a C++11 standard library? It's the same pattern used by std::make_unique,
std::make_shared, std::vector<T>::emplace_back, std::set<T>::emplace, etc.
(which are indeed called in the Google repository, and there is a template
similar to this one in that repository as well).

[jbroman, 2015-05-21 17:09:28]

(this paragraph was somehow lost from my previous message)

That said, if there's still 2xNotLGTM, I'll just go and change the
particular recorder classes instead. It just seems rather un-DRY.

[pdr., 2015-05-22 18:00:46]

On 2015/05/21 at 17:09:28, jbroman wrote:
> (this paragraph was somehow lost from my previous message)
> 
> That said, if there's still 2xNotLGTM, I'll just go and change the
> particular recorder classes instead. It just seems rather un-DRY.

I think we do want the Optional class in Blink, but I like Chris's proposal for
refactoring the recorders without Optional:
ClipRecorder clipRecorder;
if (shouldClip) {
    clipRecorder.begin(context, bounds);
    ...
}

[jbroman, 2015-05-26 17:31:25]

On 2015/05/22 18:00:46, pdr wrote:
> On 2015/05/21 at 17:09:28, jbroman wrote:
> > (this paragraph was somehow lost from my previous message)
> > 
> > That said, if there's still 2xNotLGTM, I'll just go and change the
> > particular recorder classes instead. It just seems rather un-DRY.
> 
> I think we do want the Optional class in Blink, but I like Chris's proposal
for
> refactoring the recorders without Optional:
> ClipRecorder clipRecorder;
> if (shouldClip) {
>     clipRecorder.begin(context, bounds);
>     ...
> }

I've wrote this CL (though without additional unit tests), but it's much much
larger than this one, and adds enough confusion/complexity to the recorder
classes that I'm not going to land it:
  https://codereview.chromium.org/1144203004/

Lacking any way forward that both I and reviewers are happy with, abandoning
this CL now.

[jbroman, 2015-05-29 17:45:49]

Reopening for further discussion.

[pdr., 2015-05-29 17:52:14]

On 2015/05/29 at 17:45:49, jbroman wrote:
> Reopening for further discussion.

LGTM, the perf numbers don't lie [1] and I don't think this adds much
complexity.

@esprehn, do you still have open concerns about this approach?


[1]
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/j...

[chrishtr, 2015-05-29 17:57:58]

lgtm

I discussed offline with Jeremy, and also reviewed the comments on the related
bug and this CL,
and on CL 1144203004. I think this version is cleaner. (Also, this pattern is
where C++ and chromium are going over time.)

To summarize why this is justified:

The optimization pattern here is similar to what cc::ListContainer uses. The
idea is that instead of allocating on the heap, you pre-allocate inline storage
of appropriate size, and initialize it when needed if the object needs to be
instantiated.  In this case, the allocation is entirely on the stack so no
malloc is needed at all.
Vector with inline capacity is another similar example.

This has a significant performance impact on slimming paint, because of the very
high cost of malloc, especially on Android.

[jbroman, 2015-06-01 18:27:41]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2015-06-01 18:28:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129793005/40001

[commit-bot: I haz the power, 2015-06-01 18:31:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-01 18:32:01]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/57141)

[jbroman, 2015-06-01 19:50:49]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2015-06-01 19:50:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1129793005/#ps60001
(title: "merge with master")

[commit-bot: I haz the power, 2015-06-01 19:51:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129793005/60001

[commit-bot: I haz the power, 2015-06-01 23:35:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-01 23:35:50]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[jbroman, 2015-06-02 00:23:59]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2015-06-02 00:25:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129793005/60001

[commit-bot: I haz the power, 2015-06-02 01:23:41]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196267

[esprehn, 2015-06-02 02:00:22]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org, pdr@chromium.org

[esprehn, 2015-06-02 02:00:23]

https://codereview.chromium.org/1129793005/diff/60001/Source/wtf/Optional.h
File Source/wtf/Optional.h (right):

https://codereview.chromium.org/1129793005/diff/60001/Source/wtf/Optional.h#n...
Source/wtf/Optional.h:45: T& operator*() { ASSERT(m_engaged); return *storage();
}
These should all be ASSERT_WITH_SECURITY_IMPLICATION since this uses garbage
memory if you get here.

[jbroman, 2015-06-02 14:13:49]

https://codereview.chromium.org/1129793005/diff/60001/Source/wtf/Optional.h
File Source/wtf/Optional.h (right):

https://codereview.chromium.org/1129793005/diff/60001/Source/wtf/Optional.h#n...
Source/wtf/Optional.h:45: T& operator*() { ASSERT(m_engaged); return *storage();
}
On 2015/06/02 02:00:23, esprehn wrote:
> These should all be ASSERT_WITH_SECURITY_IMPLICATION since this uses garbage
> memory if you get here.

Right you are. https://codereview.chromium.org/1159623012