Clean up EGL thread state at thread exit in nanobench and DM

Clean up EGL thread state at thread exit in nanobench and DM

Call eglReleaseThread for each thread that might have executed
EGL code.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1733323002

[Kimmo Kinnunen, 2016-02-25 12:50:57]

Description was changed from

==========
Clean up EGL thread state at thread exit in nanobench and DM

Call eglReleaseThread for each thread that might have executed
EGL code.

BUG=skia:
==========

to

==========
Clean up EGL thread state at thread exit in nanobench and DM

Call eglReleaseThread for each thread that might have executed
EGL code.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Kimmo Kinnunen, 2016-02-25 12:51:37]

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

[Kimmo Kinnunen, 2016-02-25 12:51:38]

Would this be overkill?

[mtklein, 2016-02-25 14:18:45]

Why?

These threads all only terminate when the process is about to die.

[Kimmo Kinnunen, 2016-02-25 14:39:30]

On 2016/02/25 14:18:45, mtklein wrote:
> Why?
> 
> These threads all only terminate when the process is about to die.

For avoiding leaks, so that legit leaks would be easier to detect? I believe
other code does deallocation when the process shuts down..

[mtklein, 2016-02-25 14:40:56]

On 2016/02/25 14:39:30, Kimmo Kinnunen wrote:
> On 2016/02/25 14:18:45, mtklein wrote:
> > Why?
> > 
> > These threads all only terminate when the process is about to die.
> 
> For avoiding leaks, so that legit leaks would be easier to detect? I believe
> other code does deallocation when the process shuts down..

Are we suppressing these leaks today?  What tool catches these leaks?

[Kimmo Kinnunen, 2016-02-25 15:00:23]

On 2016/02/25 14:40:56, mtklein wrote:
> On 2016/02/25 14:39:30, Kimmo Kinnunen wrote:
> > On 2016/02/25 14:18:45, mtklein wrote:
> > > Why?
> > > 
> > > These threads all only terminate when the process is about to die.
> > 
> > For avoiding leaks, so that legit leaks would be easier to detect? I believe
> > other code does deallocation when the process shuts down..
> 
> Are we suppressing these leaks today?  What tool catches these leaks?

Valgrind should catch them, I do not know if *SAN does.
I have not checked all the suppressions, I can do that tomorrow. Likely there
are suppressions, as valgrind bots are working. 
There are missing suppressions, such as for command buffer egl implementation.

[mtklein, 2016-02-25 15:26:02]

Let's make sure we've got the problems positively identified before we fix them.

[Kimmo Kinnunen, 2016-02-26 10:31:26]

On 2016/02/25 15:26:02, mtklein wrote:
> Let's make sure we've got the problems positively identified before we fix
them.

What do you mean? As far as I understand EGL will leak if eglReleaseThread() is
not called.

There are no suppressions. Maybe the developers do not care that it leaks at the
moment?

EGL on Android probably leaks before this. Depending on the driver quality, most
likely after this but maybe less. This is not so visible, as valgrind is
probably not run on android (I've not checked if valgrind on android is usable).

ANGLE on desktop leaks before and after this. I think it's a bug in ANGLE where
eglReleaseThread is not implemented correctly. After this, ANGLE could be fixed.

Command buffer on desktop leaks various chromium global objects before and after
this. This removes the non-global leaks that come from not cleaning up EGL
interface.

EGL on desktop is not runnable at the moment.

[mtklein, 2016-02-26 13:16:41]

On 2016/02/26 10:31:26, Kimmo Kinnunen wrote:
> On 2016/02/25 15:26:02, mtklein wrote:
> > Let's make sure we've got the problems positively identified before we fix
> them.
> 
> What do you mean? As far as I understand EGL will leak if eglReleaseThread()
is
> not called.
> 
> There are no suppressions. Maybe the developers do not care that it leaks at
the
> moment?

Yes, exactly.  Let's solve problems we have, not problems we might have later.
Future-us will know better how or if we want to test EGL thread state leaks.
Let's let future-us solve the problem.

> EGL on Android probably leaks before this. Depending on the driver quality,
most
> likely after this but maybe less. This is not so visible, as valgrind is
> probably not run on android (I've not checked if valgrind on android is
usable).

Right.  To my knowledge no bots are running Valgrind on Android.
It seems possible, but I've got to imagine it'd be slow to the point of
uselessness.
 
> ANGLE on desktop leaks before and after this. I think it's a bug in ANGLE
where
> eglReleaseThread is not implemented correctly. After this, ANGLE could be
fixed.
> 
> Command buffer on desktop leaks various chromium global objects before and
after
> this. This removes the non-global leaks that come from not cleaning up EGL
> interface.
> 
> EGL on desktop is not runnable at the moment.

We only care about these leaks in test tools if something is going to shim in
just before process exit and report those leaks.
Today that's Valgrind, LSAN, or DrMemory.  If you're looking to set up more of
these bots, that's fine, but this sort of change
should follow those new bots, not precede it.  I don't want to land code
complexity that is not yet outweighed by some
current concrete benefit.

If we want to land something like this, let's first actually make a bot go red
because it detects the leak.

[Kimmo Kinnunen, 2016-02-26 14:44:23]

Well, it is a problem for current me. And I believe future me would fix it the
same way. Judging from the results, it would make sense to run angle and cmdbuf
on valgrind bot, too. I can not make it happen, though, so I guess we can
abandon this.

[Kimmo Kinnunen, 2016-02-26 14:52:36]

Description was changed from

==========
Clean up EGL thread state at thread exit in nanobench and DM

Call eglReleaseThread for each thread that might have executed
EGL code.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Clean up EGL thread state at thread exit in nanobench and DM

Call eglReleaseThread for each thread that might have executed
EGL code.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[mtklein, 2016-02-26 14:53:55]

On 2016/02/26 14:44:23, Kimmo Kinnunen wrote:
> Well, it is a problem for current me. And I believe future me would fix it the
> same way. Judging from the results, it would make sense to run angle and
cmdbuf
> on valgrind bot, too. I can not make it happen, though, so I guess we can
> abandon this.

Everyone likes to think that they will handle things in the future the same way
they would handle them today.  This is a good approximation but not always
true.  Future people have more information.  Here we're trying to solve a
problem
speculatively that future people can solve with actual knowledge of the problem.


Please help me understand why this is a problem for you today.
What work can you not do because the EGL thread state leaks?

By "I can not make it happen", do you mean you cannot run angle and cmdbuf
configs in Valgrind?
If so, what's blocking you?  Does cmdbuf run on Linux?  What about angle?

[bsalomon, 2016-02-26 15:04:14]

This seems like putting the cart before the horse. We don't even have a regular
command buffer bot yet, though hopefully will in the near future. The tool
itself leaking isn't itself a problem as the tool only exists to test the
library. If and when we have *san or valgrind bot for the command buffer or
angle we can address tool leaks that cloud the view of leaks in the
library/command buffer or simply suppress them. When a new bot goes up it's ok
for it to be red while these types of issues are sorted out. At that time we
will have the bot to test the fixes and detect regressions.

[mtklein, 2016-02-26 15:12:33]

On 2016/02/26 15:04:14, bsalomon wrote:
> This seems like putting the cart before the horse. We don't even have a
regular
> command buffer bot yet, though hopefully will in the near future. The tool
> itself leaking isn't itself a problem as the tool only exists to test the
> library. If and when we have *san or valgrind bot for the command buffer or
> angle we can address tool leaks that cloud the view of leaks in the
> library/command buffer or simply suppress them. When a new bot goes up it's ok
> for it to be red while these types of issues are sorted out. At that time we
> will have the bot to test the fixes and detect regressions.

👍