Prototype for less memory intensive initializing of builtins.

Less memory intensive initializing of builtins.

R=alokp@chromium.org

Committed: https://code.google.com/p/angleproject/source/detail?r=2425

[Alan Leung Chromium, 2013-05-15 21:45:14]

Hey Alok

I played around with the idea of not using the parser at all for builtin
initialization.

I tried to maintain the current structure in order to keep them easy to update.
Again, I am not too comfortable with C++ so I am welcome to other suggestions
besides macros.

I have redo the measurements in Android again. It saves almost exactly the same
amount of memory as the early table copying hack I sent out. 

It is probably not surprising that the runtime difference is insignificant.

What's your take on this one?

[Alok Priyadarshi, 2013-05-16 04:19:37]

A couple of questions. I am not sure where the memory savings are coming from.
See my comments below.

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp
File src/compiler/Initialize.cpp (left):

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#...
src/compiler/Initialize.cpp:24: TString s;
The question is why did the memory usage reduce? I can think of two reasons:
1. We are not building the AST for common functions. I doubt this because these
functions should not be placed into AST.
2. We are not building this huge TString, which allocates the memory for the
internal char buffer using the GlobalPoolAllocator.

I think it is most probably due to 2. If this is the case, this can be very
easily fixed by using std::String instead of TSring. Can you use std::string for
builtInStrings instead of TString and see if you get similar savings?

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp
File src/compiler/Initialize.cpp (right):

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#...
src/compiler/Initialize.cpp:836:
//builtInStrings.push_back(BuiltInFunctionsCommon(resources));
Why only build it for common functions? What is preventing us from doing it for
others?

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h
File src/compiler/Initialize.h (right):

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h#ne...
src/compiler/Initialize.h:38: TType * t = 0;
If you directly used the TBasicType enum, there would not be any need for string
matching.

[Alok Priyadarshi, 2013-05-16 04:19:38]

A couple of questions. I am not sure where the memory savings are coming from.
See my comments below.

[nicolas%transgaming.com, 2013-05-16 14:13:13]

Please note that this code is undergoing significant changes for ES3 support,
since it defines new shader builtins. These patches will be merged into the
es3proto branch in a couple of weeks.

That said I definitely like the idea of inserting the builtin signatures into
the symbol table directly instead of by parsing them!

Could you replace the macros with inline functions? Thanks.

[Alan Leung Chromium, 2013-05-16 18:05:49]

Thanks for looking at this, Alok / Nicolas.

As for nicolas's question: I thought about functions before. What is the
advantage over macros? I went with macros because I can get away with not
quoting the every parameter as a string. It sort of resembles the original code
if you squint hard enough.

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp
File src/compiler/Initialize.cpp (left):

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#...
src/compiler/Initialize.cpp:24: TString s;
On 2013/05/16 04:19:37, Alok Priyadarshi wrote:
> The question is why did the memory usage reduce? I can think of two reasons:
> 1. We are not building the AST for common functions. I doubt this because
these
> functions should not be placed into AST.
> 2. We are not building this huge TString, which allocates the memory for the
> internal char buffer using the GlobalPoolAllocator.
> 
> I think it is most probably due to 2. If this is the case, this can be very
> easily fixed by using std::String instead of TSring. Can you use std::string
for
> builtInStrings instead of TString and see if you get similar savings?

I think it is a bit of both according to the profiling I have done. I have seen
lots of pages getting allocated for both the strings as well as artifacts needed
by the parser. Looking at the profile sometimes isn't very telling thought,
because I only see where new pages needed to be allocated but not the real
distribution of objects.

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h
File src/compiler/Initialize.h (right):

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h#ne...
src/compiler/Initialize.h:38: TType * t = 0;
On 2013/05/16 04:19:37, Alok Priyadarshi wrote:
> If you directly used the TBasicType enum, there would not be any need for
string
> matching.

I still need to know the dimension of the vector as well as setting
matrix=true/false right?

[Alok Priyadarshi, 2013-05-16 22:51:11]

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp
File src/compiler/Initialize.cpp (left):

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#...
src/compiler/Initialize.cpp:24: TString s;
Can you try just using std::string instead of TString for this and check how
much saving you get?

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp
File src/compiler/Initialize.cpp (right):

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#...
src/compiler/Initialize.cpp:836:
//builtInStrings.push_back(BuiltInFunctionsCommon(resources));
On 2013/05/16 04:19:37, Alok Priyadarshi wrote:
> Why only build it for common functions? What is preventing us from doing it
for
> others?

Any reason you are only doing it for common functions?

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h
File src/compiler/Initialize.h (right):

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h#ne...
src/compiler/Initialize.h:38: TType * t = 0;
quite right. I am sure we can come up with a maintainable and efficient
mechanism to do this mapping. I would first like to understand how we are able
to save that much memory.

[Alan Leung Chromium, 2013-05-20 22:26:53]

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp
File src/compiler/Initialize.cpp (left):

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#...
src/compiler/Initialize.cpp:24: TString s;
On 2013/05/16 22:51:11, Alok Priyadarshi wrote:
> Can you try just using std::string instead of TString for this and check how
> much saving you get?

Turned out to be very little. My guess is that the declaration code is under
8000 chars and not very big compared to the AST.

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp
File src/compiler/Initialize.cpp (right):

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.cpp#...
src/compiler/Initialize.cpp:836:
//builtInStrings.push_back(BuiltInFunctionsCommon(resources));
On 2013/05/16 22:51:11, Alok Priyadarshi wrote:
> On 2013/05/16 04:19:37, Alok Priyadarshi wrote:
> > Why only build it for common functions? What is preventing us from doing it
> for
> > others?
> 
> Any reason you are only doing it for common functions?

There is no real reason, just laziness :)

I just wanted to get a quick proof of concept to work and see what kind of
numbers we get from it.

Just wanted to avoid wasted work if people don't feel this is a good change :)

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h
File src/compiler/Initialize.h (right):

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h#ne...
src/compiler/Initialize.h:38: TType * t = 0;
On 2013/05/16 22:51:11, Alok Priyadarshi wrote:
> quite right. I am sure we can come up with a maintainable and efficient
> mechanism to do this mapping. I would first like to understand how we are able
> to save that much memory.

See earlier comment in this patch. The AST is currently pretty heavy on memory.
Similar to my earlier SymbolTable cloning suggestion, we only need the
symboltable that it generates and the AST make up around ~0.3MB. That's
something we never need again.

[Alok Priyadarshi, 2013-05-21 00:06:12]

I like the idea of manually building the built-in symbol table. Lets figure out
how to do it in a easily extensible and efficient manner. I propose a
combination of function dictionary and python script.

Nicolas: Do you have any other suggestions?

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h
File src/compiler/Initialize.h (right):

https://codereview.appspot.com/9436044/diff/1004/src/compiler/Initialize.h#ne...
src/compiler/Initialize.h:89: #define BUILTIN1(t, rvalue, name, ptype1, pname1) 
\
I propose auto-generating a cpp file with these function calls. We could have a
human-readable dictionary of all built-in functions and a python script that
spits out these calls.

We can discuss this in person if you want.

[Alan Leung Chromium, 2013-05-23 21:53:50]

> combination of function dictionary and python script.
> 
> Nicolas: Do you have any other suggestions?

I basically wrote a Perl script to generate those function calls.

I am happy to port it to Python. Should I bring this up in the mailing list and
see what other think?

[Alok Priyadarshi, 2013-05-23 22:38:12]

Yes please port it to python if you can.

[Alan Leung Chromium, 2013-05-28 18:18:42]

On 2013/05/23 22:38:12, Alok Priyadarshi wrote:
> Yes please port it to python if you can.


What do you mean by a function dictionary?

I am thinking of a text file just with a similar structure as this:


//
// Angle and Trigonometric Functions.
//
float radians(float, degrees)
vec2 radians(vec2, degrees)
vec3 radians(vec3, degrees)
vec4 radians(vec4, degrees)

float degrees(float, radians)
vec2 degrees(vec2, radians)
vec3 degrees(vec3, radians)
vec4 degrees(vec4, radians)

float sin(float, angle)
vec2 sin(vec2, angle)
vec3 sin(vec3, angle)
vec4 sin(vec4, angle)

.....

THoughts?

[Alok Priyadarshi, 2013-05-28 19:16:34]

If you use a text file, you would need to parse it. I was thinking of a
hard-coded python dictionary:

[
  {
    name: "radians",
    return_type: "float",
    parameters: [
      type: "float",
      name: "degrees"
    ]
  },
  ...
]

Manually adding/removing entries from such a dictionary should be pretty easy
and writing/maintaining the code to spit-out code to insert these entries into
the symbol table would be straight-forward.

[Alan Leung Chromium, 2013-05-28 21:43:05]

What do you think of something like this.

It allows us to have finer control such as putting "if statements" in the
definition file.

[Alok Priyadarshi, 2013-05-28 21:50:01]

https://codereview.appspot.com/9436044/diff/18002/src/compiler/Initialize.h
File src/compiler/Initialize.h (right):

https://codereview.appspot.com/9436044/diff/18002/src/compiler/Initialize.h#n...
src/compiler/Initialize.h:37: static TType* GetGlobalType(const std::string&
name) {
One of the goals of auto-generating the code to populate symbol-table is to not
have these macros or string-matching during runtime. This should be resolved
inside the script.

[Alan Leung Chromium, 2013-05-28 22:18:13]

On 2013/05/28 21:50:01, Alok Priyadarshi wrote:
> https://codereview.appspot.com/9436044/diff/18002/src/compiler/Initialize.h
> File src/compiler/Initialize.h (right):
> 
>
https://codereview.appspot.com/9436044/diff/18002/src/compiler/Initialize.h#n...
> src/compiler/Initialize.h:37: static TType* GetGlobalType(const std::string&
> name) {
> One of the goals of auto-generating the code to populate symbol-table is to
not
> have these macros or string-matching during runtime. This should be resolved
> inside the script.

Makes total sense. Let me work on that.

But what do you think about the common.input file format? Does this sound like a
good approach to you?

[Alok Priyadarshi, 2013-05-28 22:29:09]

> But what do you think about the common.input file format? Does this sound like
a
> good approach to you?

I would prefer that the file format easily supports all types that can be
inserted into symbol-table, i.e., type-names, variables, and functions. If the
proposed file-format supports it, I am fine with that. It seems like to support
all three, the parser would become a bit involved.

[Alan Leung Chromium, 2013-05-29 00:32:59]

On 2013/05/28 22:29:09, Alok Priyadarshi wrote:
> > But what do you think about the common.input file format? Does this sound
like
> a
> > good approach to you?
> 
> I would prefer that the file format easily supports all types that can be
> inserted into symbol-table, i.e., type-names, variables, and functions. If the
> proposed file-format supports it, I am fine with that. It seems like to
support
> all three, the parser would become a bit involved.

How about now?

[Alok Priyadarshi, 2013-05-29 05:43:19]

Getting closer!

I think the following scheme would work well:

1. builtin_symbols.json will contain a dictionary of all built-in symbols -
types, variables, and functions
2. generate_builtin_symbol_table.py will parse builtin_symbols.json and produce
builtin_symbol_table.[h,cpp]
3. builtin_symbol_table.[h,cpp] will contain function definition for inserting
built-in symbols:
InsertBuiltInSymbols(ShShaderType type,
                     ShShaderSpec spec,
                     const ShBuiltInResources& resources,
                     TSymbolTable* symbolTable);

https://codereview.appspot.com/9436044/diff/24001/src/compiler/Initialize.h
File src/compiler/Initialize.h (right):

https://codereview.appspot.com/9436044/diff/24001/src/compiler/Initialize.h#n...
src/compiler/Initialize.h:37: #define BUILTIN1(t, rvalue, name, ptype1, pname1) 
\
Is there a reason for these macros to be here? Can they moved into the script as
well?

[Alan Leung Chromium, 2013-05-29 23:25:22]

Thanks for the suggestions, Alok. Would you mind taking another look?

On 2013/05/29 05:43:19, Alok Priyadarshi wrote:
> Getting closer!
> 
> I think the following scheme would work well:
> 
> 1. builtin_symbols.json will contain a dictionary of all built-in symbols -
> types, variables, and functions

Done.


> 2. generate_builtin_symbol_table.py will parse builtin_symbols.json and
produce
> builtin_symbol_table.[h,cpp]

Done.


> 3. builtin_symbol_table.[h,cpp] will contain function definition for inserting
> built-in symbols:
> InsertBuiltInSymbols(ShShaderType type,
>                      ShShaderSpec spec,
>                      const ShBuiltInResources& resources,
>                      TSymbolTable* symbolTable);
> 
> https://codereview.appspot.com/9436044/diff/24001/src/compiler/Initialize.h
> File src/compiler/Initialize.h (right):
> 
>
https://codereview.appspot.com/9436044/diff/24001/src/compiler/Initialize.h#n...
> src/compiler/Initialize.h:37: #define BUILTIN1(t, rvalue, name, ptype1,
pname1) 
> \
> Is there a reason for these macros to be here? Can they moved into the script
as
> well?

I choose to have the C preprocessing explan the macros instead of me expanding
it in the function. Or are you just suggesting that I put the definition in a
generated file? I am not seeing a clear advantage.

Thanks!

-Alan

[Alok Priyadarshi, 2013-05-30 18:49:47]

did you forget to include the json file?

https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.cpp (right):

https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.cpp:1: // Copyright (c) 2002-2013 The ANGLE
Project Authors. All rights reserved.
Add a warning that this file is auto-generated. DO NOT EDIT.

https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.h (right):

https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.h:11: #define BUILTIN1(t, rvalue, name,
ptype1, pname1)  \
These macros should be moved to builtin_symbol_table.cpp. They are not used
elsewhere.

https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.h:40: void InsertBuiltInFunctionsCommon(const
ShBuiltInResources& resources, TSymbolTable * t);
This function should be extern.

[Alan Leung Chromium, 2013-05-30 20:18:48]

I added the json file as well.

https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.cpp (right):

https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.cpp:1: // Copyright (c) 2002-2013 The ANGLE
Project Authors. All rights reserved.
On 2013/05/30 18:49:48, Alok Priyadarshi wrote:
> Add a warning that this file is auto-generated. DO NOT EDIT.

Done.

https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.h (right):

https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.h:11: #define BUILTIN1(t, rvalue, name,
ptype1, pname1)  \
On 2013/05/30 18:49:48, Alok Priyadarshi wrote:
> These macros should be moved to builtin_symbol_table.cpp. They are not used
> elsewhere.

Good Point. Done.

https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.h:40: void InsertBuiltInFunctionsCommon(const
ShBuiltInResources& resources, TSymbolTable * t);
On 2013/05/30 18:49:48, Alok Priyadarshi wrote:
> This function should be extern.

Done.

[Alok Priyadarshi, 2013-05-31 03:47:57]

Almost there.

Did you forget to include changes to avoid adding common-function string?

https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.h (right):

https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.h:9: #include "compiler/SymbolTable.h"
No need to include SymbolTable.h anymore. Just fwd-declare ShBuiltInResources
and SymbolTable.

https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.h:11: 
nit: extra line.

https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbols.json (right):

https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol...
src/compiler/builtin_symbols.json:24: "common" : [
I think it is fine with the scope of your changes.

But if we need to move over all built-in symbols, the dictionary needs to
support all three types - built-in types, variables, and functions. This can be
added in subsequent patches.

[Alan Leung Chromium, 2013-06-03 20:40:51]

PTAL?

> Did you forget to include changes to avoid adding common-function string?

What do you mean? I don't fully follow.

Do you mean that CL I have to avoid creating too much TStrings for mangled
names? That's orthogonal to this work, no?

[Alan Leung Chromium, 2013-06-03 20:41:57]

PTAL?

https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.h (right):

https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.h:9: #include "compiler/SymbolTable.h"
On 2013/05/31 03:47:58, Alok Priyadarshi wrote:
> No need to include SymbolTable.h anymore. Just fwd-declare ShBuiltInResources
> and SymbolTable.

Done.

https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.h:11: 
On 2013/05/31 03:47:58, Alok Priyadarshi wrote:
> nit: extra line.

Done.

https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbols.json (right):

https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol...
src/compiler/builtin_symbols.json:24: "common" : [
On 2013/05/31 03:47:58, Alok Priyadarshi wrote:
> I think it is fine with the scope of your changes.
> 
> But if we need to move over all built-in symbols, the dictionary needs to
> support all three types - built-in types, variables, and functions. This can
be
> added in subsequent patches.

Totally agree.

I have no plan to stop working on this after this CL. But I find smaller patches
easier to work with.

[Alok Priyadarshi, 2013-06-03 20:46:32]

On 2013/06/03 20:40:51, acleung1 wrote:
> PTAL?
> 
> > Did you forget to include changes to avoid adding common-function string?
> 
> What do you mean? I don't fully follow.
> 
> Do you mean that CL I have to avoid creating too much TStrings for mangled
> names? That's orthogonal to this work, no?

I do not understand how this works if you still call BuiltInFunctionsCommon() in
Initialize.cpp. I would expect that function to go away. Otherwise
InsertBuiltInFunctionsCommon() will not do anything.

I do not see any changes to Initialize.cpp in this patch. You had call to
BuiltInFunctionsCommon commented out in earlier patches.

[nicolas, 2013-06-04 19:51:32]

Please note that using a macro will create a lot of code. Also, this approach is
creating the same TType many times. You could instead have a new symbol table
method that looks like this:

bool TSymbolTable::insertBuiltIn(TType *rvalue, const char *name, TType *ptype1,
const char *pname1, TType *ptype2 = 0,
                                 const char *pname2 = 0, TType *ptype3 = 0,
const char *pname3 = 0)
{
    TFunction *function = new TFunction(NewPoolTString(name), *rvalue);

    TParameter param1 = {NewPoolTString(pname1), ptype1};
    function->addParameter(param1);

    if (pname2)
    {
        TParameter param2 = {NewPoolTString(pname2), ptype2};
        function->addParameter(param2);
    }

    if (pname3)
    {
        TParameter param3 = {NewPoolTString(pname3), ptype3};
        function->addParameter(param3);
    }

    return insert(*function);
}

And use it like this:

    TType *float1 = new TType(EbtFloat, EbpUndefined, EvqGlobal, 1);
    TType *float2 = new TType(EbtFloat, EbpUndefined, EvqGlobal, 2);
    ...

    symbolTable.insertBuiltIn(float1, "radians", float1, "degrees");
    symbolTable.insertBuiltIn(float2, "radians", float2, "degrees");
    ...

This also eliminates the need for the json file and the python script.

Just a suggestion.

[Alan Leung Chromium, 2013-06-04 23:32:57]

On 2013/06/03 20:46:32, Alok Priyadarshi wrote:
> On 2013/06/03 20:40:51, acleung1 wrote:
> > PTAL?
> > 
> > > Did you forget to include changes to avoid adding common-function string?
> > 
> > What do you mean? I don't fully follow.
> > 
> > Do you mean that CL I have to avoid creating too much TStrings for mangled
> > names? That's orthogonal to this work, no?
> 
> I do not understand how this works if you still call BuiltInFunctionsCommon()
in
> Initialize.cpp. I would expect that function to go away. Otherwise
> InsertBuiltInFunctionsCommon() will not do anything.
> 
> I do not see any changes to Initialize.cpp in this patch. You had call to
> BuiltInFunctionsCommon commented out in earlier patches.

Ah I am missing Initialize.cpp. I added it back. Please take another look?

How do you normally work on Angle? I use git for Chromium and manually copy my
changes to a Angle SVN client. I ended up forgetting lots of files over time.

[Alok Priyadarshi, 2013-06-05 21:16:00]

On 2013/06/04 23:32:57, acleung1 wrote:
> On 2013/06/03 20:46:32, Alok Priyadarshi wrote:
> > On 2013/06/03 20:40:51, acleung1 wrote:
> > > PTAL?
> > > 
> > > > Did you forget to include changes to avoid adding common-function
string?
> > > 
> > > What do you mean? I don't fully follow.
> > > 
> > > Do you mean that CL I have to avoid creating too much TStrings for mangled
> > > names? That's orthogonal to this work, no?
> > 
> > I do not understand how this works if you still call
BuiltInFunctionsCommon()
> in
> > Initialize.cpp. I would expect that function to go away. Otherwise
> > InsertBuiltInFunctionsCommon() will not do anything.
> > 
> > I do not see any changes to Initialize.cpp in this patch. You had call to
> > BuiltInFunctionsCommon commented out in earlier patches.
> 
> Ah I am missing Initialize.cpp. I added it back. Please take another look?
> 
> How do you normally work on Angle? I use git for Chromium and manually copy my
> changes to a Angle SVN client. I ended up forgetting lots of files over time.

I have the ANGLE client inside chromium checkout. You first tell gclient to not
pull ANGLE. You .gclient should look like this:

solutions = [
  { "name"        : "src",
    "url"         : "https://chromium.googlesource.com/chromium/src.git",
    "deps_file"   : ".DEPS.git",
    "managed"     : True,
    "custom_deps" : {
      "src/third_party/angle": None,
    },
  },
]

Notice the entry for angle inside custom_deps. Then "cd src/third_party; git svn
clone https://angleproject.googlecode.com/svn/trunk angle". Start hacking under
src/third_party/angle. Upload patches using "git cl upload".

[Alok Priyadarshi, 2013-06-05 21:21:30]

On 2013/06/04 19:51:32, nicolas wrote:
> Please note that using a macro will create a lot of code. Also, this approach
is
> creating the same TType many times. You could instead have a new symbol table
> method that looks like this:
> 
> bool TSymbolTable::insertBuiltIn(TType *rvalue, const char *name, TType
*ptype1,
> const char *pname1, TType *ptype2 = 0,
>                                  const char *pname2 = 0, TType *ptype3 = 0,
> const char *pname3 = 0)
> {
>     TFunction *function = new TFunction(NewPoolTString(name), *rvalue);
> 
>     TParameter param1 = {NewPoolTString(pname1), ptype1};
>     function->addParameter(param1);
> 
>     if (pname2)
>     {
>         TParameter param2 = {NewPoolTString(pname2), ptype2};
>         function->addParameter(param2);
>     }
> 
>     if (pname3)
>     {
>         TParameter param3 = {NewPoolTString(pname3), ptype3};
>         function->addParameter(param3);
>     }
> 
>     return insert(*function);
> }
> 
> And use it like this:
> 
>     TType *float1 = new TType(EbtFloat, EbpUndefined, EvqGlobal, 1);
>     TType *float2 = new TType(EbtFloat, EbpUndefined, EvqGlobal, 2);
>     ...
> 
>     symbolTable.insertBuiltIn(float1, "radians", float1, "degrees");
>     symbolTable.insertBuiltIn(float2, "radians", float2, "degrees");
>     ...
> 
> This also eliminates the need for the json file and the python script.
> 
> Just a suggestion.

Good point about avoiding macro. I still like the json file approach because it
may be easier to maintain, especially when we add other types of built-in
symbols, some of which may only be relevant for specific shader types, spec,
language version, etc.

I think we can incorporate your suggestion for sharing TType in the script. Did
you have any other concern?

[Alok Priyadarshi, 2013-06-05 21:26:04]

https://codereview.appspot.com/9436044/diff/47001/src/compiler/Initialize.cpp
File src/compiler/Initialize.cpp (left):

https://codereview.appspot.com/9436044/diff/47001/src/compiler/Initialize.cpp...
src/compiler/Initialize.cpp:503:
builtInStrings.push_back(BuiltInFunctionsCommon(resources));
Could you also delete BuiltInFunctionsCommon if it is not used anymore?

https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.cpp (right):

https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.cpp:12: #define BUILTIN1(t, rvalue, name,
ptype1, pname1)  {   TFunction* f = new TFunction(new TString(name), *rvalue);  
TParameter param = {new TString(pname1), ptype1};   f->addParameter(param);  
t->insert(*f); }
Nicolas has a good point about not using these macros. I think this could be a
function as Nicolas described in his patch. We can work on sharing TType in
future patches.

https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.h (right):

https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.h:9: #include "GLSLANG/ShaderLang.h"
Is there a reason for including this file here?

[Alan Leung Chromium, 2013-06-05 23:26:07]

PTAL.

https://codereview.appspot.com/9436044/diff/47001/src/compiler/Initialize.cpp
File src/compiler/Initialize.cpp (left):

https://codereview.appspot.com/9436044/diff/47001/src/compiler/Initialize.cpp...
src/compiler/Initialize.cpp:503:
builtInStrings.push_back(BuiltInFunctionsCommon(resources));
On 2013/06/05 21:26:05, Alok Priyadarshi wrote:
> Could you also delete BuiltInFunctionsCommon if it is not used anymore?

Done.

https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.cpp (right):

https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.cpp:12: #define BUILTIN1(t, rvalue, name,
ptype1, pname1)  {   TFunction* f = new TFunction(new TString(name), *rvalue);  
TParameter param = {new TString(pname1), ptype1};   f->addParameter(param);  
t->insert(*f); }
On 2013/06/05 21:26:05, Alok Priyadarshi wrote:
> Nicolas has a good point about not using these macros. I think this could be a
> function as Nicolas described in his patch. We can work on sharing TType in
> future patches.

Yep. Once I got this patch submitted I'll do a profile again to see how much we
save for sharing TTypes.

https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.h (right):

https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.h:9: #include "GLSLANG/ShaderLang.h"
On 2013/06/05 21:26:05, Alok Priyadarshi wrote:
> Is there a reason for including this file here?

ShBuiltInResources is actually a typedef. I am not sure if we can forward
declare a typedef.

[Alok Priyadarshi, 2013-06-05 23:55:42]

https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.cpp (right):

https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.cpp:12: #define BUILTIN1(t, rvalue, name,
ptype1, pname1)  {   TFunction* f = new TFunction(new TString(name), *rvalue);  
TParameter param = {new TString(pname1), ptype1};   f->addParameter(param);  
t->insert(*f); }
Sure. We can work on sharing TType in a later patch. But we should still remove
these macros in this patch. It should be pretty straightforward.

[Alan Leung Chromium, 2013-06-06 22:21:11]

On 2013/06/05 23:55:42, Alok Priyadarshi wrote:
>
https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
> File src/compiler/builtin_symbol_table.cpp (right):
> 
>
https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
> src/compiler/builtin_symbol_table.cpp:12: #define BUILTIN1(t, rvalue, name,
> ptype1, pname1)  {   TFunction* f = new TFunction(new TString(name), *rvalue);
 
> TParameter param = {new TString(pname1), ptype1};   f->addParameter(param);  
> t->insert(*f); }
> Sure. We can work on sharing TType in a later patch. But we should still
remove
> these macros in this patch. It should be pretty straightforward.

PTAL.

[Alan Leung Chromium, 2013-06-06 22:21:41]

Forgot to send the comments as well.

https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
File src/compiler/builtin_symbol_table.cpp (right):

https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
src/compiler/builtin_symbol_table.cpp:12: #define BUILTIN1(t, rvalue, name,
ptype1, pname1)  {   TFunction* f = new TFunction(new TString(name), *rvalue);  
TParameter param = {new TString(pname1), ptype1};   f->addParameter(param);  
t->insert(*f); }
On 2013/06/05 23:55:42, Alok Priyadarshi wrote:
> Sure. We can work on sharing TType in a later patch. But we should still
remove
> these macros in this patch. It should be pretty straightforward.

Done.

[Alok Priyadarshi, 2013-06-06 23:26:45]

On 2013/06/06 22:21:41, acleung1 wrote:
> Forgot to send the comments as well.
> 
>
https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
> File src/compiler/builtin_symbol_table.cpp (right):
> 
>
https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol...
> src/compiler/builtin_symbol_table.cpp:12: #define BUILTIN1(t, rvalue, name,
> ptype1, pname1)  {   TFunction* f = new TFunction(new TString(name), *rvalue);
 
> TParameter param = {new TString(pname1), ptype1};   f->addParameter(param);  
> t->insert(*f); }
> On 2013/06/05 23:55:42, Alok Priyadarshi wrote:
> > Sure. We can work on sharing TType in a later patch. But we should still
> remove
> > these macros in this patch. It should be pretty straightforward.
> 
> Done.

LGTM.

Before committing please make sure that WebGL conformance tests still pass. To
verify:
1. build chrome with your ANGLE changes.
2. visit
https://www.khronos.org/registry/webgl/sdk/tests/webgl-conformance-tests.html
3. run ogles/GL/build tests - all of them should pass

[Alan Leung Chromium, 2013-06-07 00:26:09]

Committed patchset #14 manually as r2425 (presubmit successful).