Moved the global-pool-allocator to TCompiler so that all memory allocated by ...

Moved the global-pool-allocator to TCompiler so that all memory allocated by the compiler can be de-allocated. Earlier the global-pool-allocator kept accumulating memory from all compilers (symbol-table in particular). The memory was only de-allocated when gpu-process exited or ShFinalize() was called. This was a problem for Chromium which keeps the GPU process around for the browser session. Now the memory is de-allocated as soon as the compiler is deleted, which happens when a tab is closed.
BUG=58808 (crbug.com)

Committed: http://code.google.com/p/angleproject/source/detail?r=489

[Alok Priyadarshi, 2010-11-23 17:33:21]

Please review - targeted to Chrome M9.

[kbr1, 2010-11-23 17:56:34]

I defer to your judgment but a couple of things seem weird to me. Also, has this
been tested with WebGL content? In particular, the situation where we re-use the
same compiler object multiple times (reloading content, navigating between
samples hosted on the same sever)?

http://codereview.appspot.com/3280041/diff/1/src/compiler/Compiler.cpp
File src/compiler/Compiler.cpp (right):

http://codereview.appspot.com/3280041/diff/1/src/compiler/Compiler.cpp#newcode61
src/compiler/Compiler.cpp:61: SetGlobalPoolAllocator(NULL);
The setting to NULL seems suspicious. TShHandleBase's constructor seems to want
to set up the allocator at the beginning of lifetime of the compiler
(ShConstructCompiler) and always have it available. However, after the first
compile is run, TScopedPoolAllocator's destructor will set it to NULL. Are there
code paths that can re-enter the compiler aside from TCompiler::Init and
TCompiler::compile? Should TScopedPoolAllocator fetch the current allocator,
store it internally and reset it upon destruction?

http://codereview.appspot.com/3280041/diff/1/src/compiler/Compiler.cpp#newcode73
src/compiler/Compiler.cpp:73: SetGlobalPoolAllocator(&allocator);
The maintenance of the global pool allocator here seems suspicious. The compiler
seems to assume its allocator will be set up for its entire lifetime, where this
is no longer the case. It seems to me that TShHandleBase's constructor and
destructor should continue to be empty, and a TScopedPoolAllocator be
instantiated everywhere necessary to allow the compiler to perform its internal
allocations.

[Alok Priyadarshi, 2010-11-23 18:19:06]

I have tested WebGL - it seems to work as before. I also traced the construction
and destruction of compiler objects. They look in order, except for redundant
sets of compiler objects being created (not related to this CL).

http://codereview.appspot.com/3280041/diff/1/src/compiler/Compiler.cpp
File src/compiler/Compiler.cpp (right):

http://codereview.appspot.com/3280041/diff/1/src/compiler/Compiler.cpp#newcode61
src/compiler/Compiler.cpp:61: SetGlobalPoolAllocator(NULL);
On 2010/11/23 17:56:34, kbr1 wrote:
> The setting to NULL seems suspicious. TShHandleBase's constructor seems to
want
> to set up the allocator at the beginning of lifetime of the compiler
> (ShConstructCompiler) and always have it available. 

This is only because the derived classes (TCompiler, TCompilerGLSL, and
TCompilerHLSL) have member variables that requires the pool-allocator to be
available during construction. I would have preferred to not set the pool
allocator in the constructor but there was no easy way around it.

However, after the first
> compile is run, TScopedPoolAllocator's destructor will set it to NULL. Are
there
> code paths that can re-enter the compiler aside from TCompiler::Init and
> TCompiler::compile?

No. These are the only two access points which setup the correct pool-allocator
before anything else.

Should TScopedPoolAllocator fetch the current allocator,
> store it internally and reset it upon destruction?

May be but not required. The two access points (Int and Compile) are covered.

http://codereview.appspot.com/3280041/diff/1/src/compiler/Compiler.cpp#newcode73
src/compiler/Compiler.cpp:73: SetGlobalPoolAllocator(&allocator);
On 2010/11/23 17:56:34, kbr1 wrote:
> The maintenance of the global pool allocator here seems suspicious. The
compiler
> seems to assume its allocator will be set up for its entire lifetime, where
this
> is no longer the case. It seems to me that TShHandleBase's constructor and
> destructor should continue to be empty, and a TScopedPoolAllocator be
> instantiated everywhere necessary to allow the compiler to perform its
internal
> allocations.

Please see my comments above. I am not proud of the fix, but this seems like the
easiest and least invasive. Suggestions welcome. For the long term I intend to
get rid of the pool-allocator entirely.

[kbr1, 2010-11-23 18:39:20]

OK. Since this has been tested, LGTM. If it looks like there is a bug in the
WebGL implementation related to redundant creation and destruction of compiler
objects please file a bug on bugs.webkit.org.

[dgkoch, 2010-11-23 20:45:25]

I guess I'm late to the party, but LGTM too.

[Alok Priyadarshi, 2010-11-23 20:48:42]

On 2010/11/23 20:45:25, dgkoch wrote:
> I guess I'm late to the party, but LGTM too.

Hi Daniel,

Sorry I am juggling quite a few CLs trying to get them in before the Holidays
and M9 branch. Please let me know if you have any objections/suggestions. I will
address in them in another CL.

-Alok

[dgkoch, 2010-11-23 20:59:43]

> Sorry I am juggling quite a few CLs trying to get them in before the Holidays
> and M9 branch. Please let me know if you have any objections/suggestions. I
will
> address in them in another CL.

Understood. have nothing to add here though.