Simplify path allocation, clean up resources correctly

Simplify path allocation, clean up resources correctly

Simplify path id allocation in NVPR backend. Instead of using
an AVL tree of path id ranges for the first 65535 ids, use just
a simple stategy of overallocation and "bump index".

Fixes the bug where previously overallocated ids were not deleted.

The advantage is that the implementation is simple and all allocations
go through overallocation, not just the first 65535 of the 1-range
allocations.

Removes the logic where paths were cleared with setting path data to
null instead of deleting the whole path. Now deleted paths are just
deleted normally. These operations should have equivalent performance on
command buffer. Deleting the path should enable the driver to do more
maintainance.

Removes the GLNameAllocator, as it was only used for paths. In order for
it to be used for other IDs, it probably would need to be re-written to
support cleanup and arbitrary ranges. Also, the interface would probably
need to be changed to not requiring the block to be allocated before it
could be managed by the structure.
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1150243003

Committed: https://skia.googlesource.com/skia/+/702501ddca7cf9b7b941ad286a0c9aa37fda86ef

[Kimmo Kinnunen, 2015-05-26 07:58:31]

kkinnunen@nvidia.com changed reviewers:
+ bsalomon@google.com, cdalton@nvidia.com

[Kimmo Kinnunen, 2015-05-26 07:58:32]

How about this bikeshedding?

[bsalomon, 2015-05-27 02:14:21]

On 2015/05/26 07:58:32, Kimmo Kinnunen wrote:
> How about this bikeshedding?

I'm ok with it, but really I defer to Chris since I think he wrote the original
implementation.

[Chris Dalton, 2015-05-27 04:24:49]

This seems fine to me for ES contexts. Are you seeing that clearing the path was
leaving any more memory tied up than deleting it? Were you able to observe an
improvement? (I like how every path goes through overallocation now instead of
just the first 64k.)

One thing I want to point out is that the name allocator was held back by ES. ES
broke the feed-forward model when they started requiring you to call glGen*. In
Big (non-ES) OpenGL, you don't have to call glGen*, you can just pick a number
and start using it as a texture, buffer, path, etc.

I was hoping with the new Big GL backend (coming soon, hopefully!) to use the
name allocator exclusively and not even put glGen* on the interface. Then we can
create any object without stalling for a round trip to the command buffer or
dual core driver, and we can even call glDelete* when finished with the object.

So as far as deleting the name allocator goes, I guess I don't care too much
whether we delete it or keep it around, but be aware that we may still have a
future use for it.

[Kimmo Kinnunen, 2015-06-01 06:20:40]

On 2015/05/27 04:24:49, Chris Dalton wrote:
> This seems fine to me for ES contexts. Are you seeing that clearing the path
was
> leaving any more memory tied up than deleting it? Were you able to observe an
> improvement? (I like how every path goes through overallocation now instead of
> just the first 64k.)

Well, the biggest thing is that it fixes the path id leaks. E.g. it is quite
discomforting to see cleanup code be intended to cleanup the state fully, but
then not doing that. Rises too many questions, at least to me.

This actually causes a fraction of MS slowdown on some testcases. This indicates
that actually we do not do much cleanup on setting path data NULL.
Mark mentioned that setting the path data NULL just sets a dirty bit and maybe
cleans up some state -- I have not looked what it does explicitly.

In my opinion, deleting the path after use is a good way to indicate the need of
cleanup of the path data. I think trying to set the data null and expecting the
driver to transform dirty bits to cleanups after some cruical amount of nulling
is a bit of wishful thinking. I would draw an analogy of doing mallocs and then
omitting frees because it is marginally faster.

Not to mention that it is quite simple, code-wise.

> One thing I want to point out is that the name allocator was held back by ES.
ES
> broke the feed-forward model when they started requiring you to call glGen*.
In
> Big (non-ES) OpenGL, you don't have to call glGen*, you can just pick a number
> and start using it as a texture, buffer, path, etc.

In order for the code to work in this scenario, it should be re-written in a bit
different way, as commented in the commit message.

> I was hoping with the new Big GL backend (coming soon, hopefully!) to use the
> name allocator exclusively and not even put glGen* on the interface. Then we
can
> create any object without stalling for a round trip to the command buffer or
> dual core driver, and we can even call glDelete* when finished with the
object.

It is not possible due to Skia API contracts. The contract is that the framework
behaves well with code not using the framework.

In other words: the code that calls Skia might create GL names that Skia does
not know. The contract could be opt-out flagged, but I do not see that as a good
tradeoff myself.

> So as far as deleting the name allocator goes, I guess I don't care too much
> whether we delete it or keep it around, but be aware that we may still have a
> future use for it.

Of course.

[Chris Dalton, 2015-06-02 03:45:06]

The patch lgtm

On 2015/06/01 06:20:40, Kimmo Kinnunen wrote:
> It is not possible due to Skia API contracts. The contract is that the
framework
> behaves well with code not using the framework.

Right. Presumably the contract would be rewritten so that if you use the "super"
backend you need to generate names through Skia.

[bsalomon, 2015-06-02 13:45:36]

On 2015/06/02 03:45:06, Chris Dalton wrote:
> The patch lgtm
> 
> On 2015/06/01 06:20:40, Kimmo Kinnunen wrote:
> > It is not possible due to Skia API contracts. The contract is that the
> framework
> > behaves well with code not using the framework.
> 
> Right. Presumably the contract would be rewritten so that if you use the
"super"
> backend you need to generate names through Skia.

lgtm

[Kimmo Kinnunen, 2016-01-14 07:04:03]

Description was changed from

==========
Simplify path allocation, clean up resources correctly

Simplify path id allocation in NVPR backend. Instead of using
an AVL tree of path id ranges for the first 65535 ids, use just
a simple stategy of overallocation and "bump index".

Fixes the bug where previously overallocated ids were not deleted.

The advantage is that the implementation is simple and all allocations
go through overallocation, not just the first 65535 of the 1-range
allocations.

Removes the logic where paths were cleared with setting path data to
null instead of deleting the whole path. Now deleted paths are just
deleted normally. These operations should have equivalent performance on
command buffer. Deleting the path should enable the driver to do more
maintainance.

Removes the GLNameAllocator, as it was only used for paths. In order for
it to be used for other IDs, it probably would need to be re-written to
support cleanup and arbitrary ranges. Also, the interface would probably
need to be changed to not requiring the block to be allocated before it
could be managed by the structure.
==========

to

==========
Simplify path allocation, clean up resources correctly

Simplify path id allocation in NVPR backend. Instead of using
an AVL tree of path id ranges for the first 65535 ids, use just
a simple stategy of overallocation and "bump index".

Fixes the bug where previously overallocated ids were not deleted.

The advantage is that the implementation is simple and all allocations
go through overallocation, not just the first 65535 of the 1-range
allocations.

Removes the logic where paths were cleared with setting path data to
null instead of deleting the whole path. Now deleted paths are just
deleted normally. These operations should have equivalent performance on
command buffer. Deleting the path should enable the driver to do more
maintainance.

Removes the GLNameAllocator, as it was only used for paths. In order for
it to be used for other IDs, it probably would need to be re-written to
support cleanup and arbitrary ranges. Also, the interface would probably
need to be changed to not requiring the block to be allocated before it
could be managed by the structure.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Kimmo Kinnunen, 2016-01-14 07:04:37]

The CQ bit was checked by kkinnunen@nvidia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-01-14 07:04:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1150243003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1150243003/20001

[commit-bot: I haz the power, 2016-01-14 07:15:51]

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

[commit-bot: I haz the power, 2016-01-14 07:15:52]

Dry run: This issue passed the CQ dry run.

[Kimmo Kinnunen, 2016-01-14 07:25:22]

This was left out when I worked on command buffer. I'll integrate this now,
before Chromium starts to use NVPR for real.

If we want to bring the reuse back, it'd perhaps be more elegant to not have the
avl nodes be refcounted. (and other bugs in the removed code fixed, of course)
https://codereview.chromium.org/684093008/

[Kimmo Kinnunen, 2016-01-14 07:25:34]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2016-01-14 07:25:35]

The patchset sent to the CQ was uploaded after l-g-t-m from cdalton@nvidia.com,
bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1150243003/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-01-14 07:25:42]

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

[commit-bot: I haz the power, 2016-01-14 07:36:47]

Description was changed from

==========
Simplify path allocation, clean up resources correctly

Simplify path id allocation in NVPR backend. Instead of using
an AVL tree of path id ranges for the first 65535 ids, use just
a simple stategy of overallocation and "bump index".

Fixes the bug where previously overallocated ids were not deleted.

The advantage is that the implementation is simple and all allocations
go through overallocation, not just the first 65535 of the 1-range
allocations.

Removes the logic where paths were cleared with setting path data to
null instead of deleting the whole path. Now deleted paths are just
deleted normally. These operations should have equivalent performance on
command buffer. Deleting the path should enable the driver to do more
maintainance.

Removes the GLNameAllocator, as it was only used for paths. In order for
it to be used for other IDs, it probably would need to be re-written to
support cleanup and arbitrary ranges. Also, the interface would probably
need to be changed to not requiring the block to be allocated before it
could be managed by the structure.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Simplify path allocation, clean up resources correctly

Simplify path id allocation in NVPR backend. Instead of using
an AVL tree of path id ranges for the first 65535 ids, use just
a simple stategy of overallocation and "bump index".

Fixes the bug where previously overallocated ids were not deleted.

The advantage is that the implementation is simple and all allocations
go through overallocation, not just the first 65535 of the 1-range
allocations.

Removes the logic where paths were cleared with setting path data to
null instead of deleting the whole path. Now deleted paths are just
deleted normally. These operations should have equivalent performance on
command buffer. Deleting the path should enable the driver to do more
maintainance.

Removes the GLNameAllocator, as it was only used for paths. In order for
it to be used for other IDs, it probably would need to be re-written to
support cleanup and arbitrary ranges. Also, the interface would probably
need to be changed to not requiring the block to be allocated before it
could be managed by the structure.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/702501ddca7cf9b7b941ad286a0c9aa37fda86ef
==========

[commit-bot: I haz the power, 2016-01-14 07:36:47]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/702501ddca7cf9b7b941ad286a0c9aa37fda86ef