Code review - Issue 9436044: Prototype for less memory intensive initializing of builtins.https://codereview.appspot.com/2013-06-07T00:26:09+00:00rietveld
Message from unknown
2013-05-15T21:35:01+00:00Alan Leung Chromiumurn:md5:be71aee365cac7d41065d07eb8f4a2d8
Message from unknown
2013-05-15T21:37:41+00:00Alan Leung Chromiumurn:md5:edac17e21f53f7aa6229a44df41e23ba
Message from acleung@chromium.org
2013-05-15T21:45:14+00:00Alan Leung Chromiumurn:md5:dd21d2fbdb59cd909ea587ecc5ead25d
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?
Message from alokp@chromium.org
2013-05-16T04:19:37+00:00Alok Priyadarshiurn:md5:8a57970144318c470663d7350878434e
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#oldcode24
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#newcode836
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#newcode38
src/compiler/Initialize.h:38: TType * t = 0;
If you directly used the TBasicType enum, there would not be any need for string matching.
Message from alokp@chromium.org
2013-05-16T04:19:38+00:00Alok Priyadarshiurn:md5:4f6e7260f9eea158e8845a677dba485e
A couple of questions. I am not sure where the memory savings are coming from. See my comments below.
Message from nicolas%transgaming.com@gtempaccount.com
2013-05-16T14:13:13+00:00nicolas%transgaming.comurn:md5:2cefccf30bf2eb9bcee41e9c8b491dd6
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.
Message from acleung@chromium.org
2013-05-16T18:05:49+00:00Alan Leung Chromiumurn:md5:66c1ee59382fb25d9d88b8d3ccc5361f
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#oldcode24
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#newcode38
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?
Message from alokp@chromium.org
2013-05-16T22:51:11+00:00Alok Priyadarshiurn:md5:d84887549c6954b46854bb7c3f6a79e6
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#oldcode24
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#newcode836
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#newcode38
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.
Message from acleung@chromium.org
2013-05-20T22:26:53+00:00Alan Leung Chromiumurn:md5:6c99b659b4dad45f772a17710cca7312
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#oldcode24
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#newcode836
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#newcode38
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.
Message from alokp@chromium.org
2013-05-21T00:06:12+00:00Alok Priyadarshiurn:md5:6acbdc6a5c728771200ed0ca28d2c45c
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#newcode89
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.
Message from acleung@chromium.org
2013-05-23T21:53:50+00:00Alan Leung Chromiumurn:md5:9d161df4630f06d4450619b99d83b452
> 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?
Message from alokp@chromium.org
2013-05-23T22:38:12+00:00Alok Priyadarshiurn:md5:77ac28eafefb89a4fbfa7d3a27e7e4b9
Yes please port it to python if you can.
Message from acleung@chromium.org
2013-05-28T18:18:42+00:00Alan Leung Chromiumurn:md5:2a31818f9958c0aa429f5b1c6d3b130b
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?
Message from alokp@chromium.org
2013-05-28T19:16:34+00:00Alok Priyadarshiurn:md5:ee29add74549223588133774f2f010be
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.
Message from unknown
2013-05-28T21:40:52+00:00Alan Leung Chromiumurn:md5:f40151ab8743541376a32a4846c30d5e
Message from acleung@chromium.org
2013-05-28T21:43:05+00:00Alan Leung Chromiumurn:md5:5230c9c093ef69d8ed25e3768cfce394
What do you think of something like this.
It allows us to have finer control such as putting "if statements" in the definition file.
Message from alokp@chromium.org
2013-05-28T21:50:01+00:00Alok Priyadarshiurn:md5:363afd50d04e988b061952ba818079ee
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#newcode37
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.
Message from acleung@chromium.org
2013-05-28T22:18:13+00:00Alan Leung Chromiumurn:md5:4100463f7ea45767183a7762976c1ec9
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#newcode37
> 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?
Message from alokp@chromium.org
2013-05-28T22:29:09+00:00Alok Priyadarshiurn:md5:4b4dfa1f39541c51078c4e1ba16020ff
> 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.
Message from unknown
2013-05-29T00:31:47+00:00Alan Leung Chromiumurn:md5:4cabeba5caed4aeadd57b465db1ff09d
Message from acleung@chromium.org
2013-05-29T00:32:59+00:00Alan Leung Chromiumurn:md5:6ad081f26969c03c98dee55d692fcf82
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?
Message from alokp@chromium.org
2013-05-29T05:43:19+00:00Alok Priyadarshiurn:md5:a6e0c0ec8332c6694ff0473f0b7bfe6a
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#newcode37
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?
Message from unknown
2013-05-29T23:20:10+00:00Alan Leung Chromiumurn:md5:ada70f068f74fbf0df98023e1469ad34
Message from unknown
2013-05-29T23:21:41+00:00Alan Leung Chromiumurn:md5:4f7973e5f0f62658a7098f1b9f004064
Message from acleung@chromium.org
2013-05-29T23:25:22+00:00Alan Leung Chromiumurn:md5:faf97ce2b215eb1d81bf4a182aa3504d
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#newcode37
> 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
Message from alokp@chromium.org
2013-05-30T18:49:47+00:00Alok Priyadarshiurn:md5:a63db2ce0a3a60ac452b6a37e3e73136
did you forget to include the json file?
https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol_table.cpp
File src/compiler/builtin_symbol_table.cpp (right):
https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol_table.cpp#newcode1
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_table.h
File src/compiler/builtin_symbol_table.h (right):
https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol_table.h#newcode11
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_table.h#newcode40
src/compiler/builtin_symbol_table.h:40: void InsertBuiltInFunctionsCommon(const ShBuiltInResources& resources, TSymbolTable * t);
This function should be extern.
Message from unknown
2013-05-30T20:17:00+00:00Alan Leung Chromiumurn:md5:d2670b8907c92e9295befd560b726072
Message from acleung@chromium.org
2013-05-30T20:18:48+00:00Alan Leung Chromiumurn:md5:be0283ea10b13a8a528f8b372816987d
I added the json file as well.
https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol_table.cpp
File src/compiler/builtin_symbol_table.cpp (right):
https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol_table.cpp#newcode1
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_table.h
File src/compiler/builtin_symbol_table.h (right):
https://codereview.appspot.com/9436044/diff/27001/src/compiler/builtin_symbol_table.h#newcode11
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_table.h#newcode40
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.
Message from alokp@chromium.org
2013-05-31T03:47:57+00:00Alok Priyadarshiurn:md5:1ef9ce57d96085ee4b8ad1ed3774eef0
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_table.h
File src/compiler/builtin_symbol_table.h (right):
https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol_table.h#newcode9
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_table.h#newcode11
src/compiler/builtin_symbol_table.h:11:
nit: extra line.
https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbols.json
File src/compiler/builtin_symbols.json (right):
https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbols.json#newcode24
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.
Message from acleung@chromium.org
2013-06-03T20:40:51+00:00Alan Leung Chromiumurn:md5:801c2864d01ed07750d80c100fd617d6
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?
Message from unknown
2013-06-03T20:41:27+00:00Alan Leung Chromiumurn:md5:a856331a05102186060a256ea91d7d90
Message from acleung@chromium.org
2013-06-03T20:41:57+00:00Alan Leung Chromiumurn:md5:121d1493e8cfcdf7fa8d430a9d2f948e
PTAL?
https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol_table.h
File src/compiler/builtin_symbol_table.h (right):
https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbol_table.h#newcode9
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_table.h#newcode11
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_symbols.json
File src/compiler/builtin_symbols.json (right):
https://codereview.appspot.com/9436044/diff/32001/src/compiler/builtin_symbols.json#newcode24
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.
Message from alokp@chromium.org
2013-06-03T20:46:32+00:00Alok Priyadarshiurn:md5:852d2eb4c21b3c67c2e47b6182e9eee6
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.
Message from nicolas@transgaming.com
2013-06-04T19:51:32+00:00nicolasurn:md5:31d04be6d2f0e8fec6d5549c7510f04c
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.
Message from unknown
2013-06-04T23:29:53+00:00Alan Leung Chromiumurn:md5:08674927f0410755caeb29f4984d05d4
Message from unknown
2013-06-04T23:30:34+00:00Alan Leung Chromiumurn:md5:da8b349afb5b328e4a78e94d11c8b50f
Message from acleung@chromium.org
2013-06-04T23:32:57+00:00Alan Leung Chromiumurn:md5:3e327c1ccbd10d0d52b97db67ab96a20
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.
Message from alokp@chromium.org
2013-06-05T21:16:00+00:00Alok Priyadarshiurn:md5:2ad30db8ea4cf9d4ea6bae224d62109d
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".
Message from alokp@chromium.org
2013-06-05T21:21:30+00:00Alok Priyadarshiurn:md5:24d84f4661215bddf4ce039abc460cf5
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?
Message from alokp@chromium.org
2013-06-05T21:26:04+00:00Alok Priyadarshiurn:md5:40fd20214c2de4f6e5c94802827b1f24
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#oldcode503
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_table.cpp
File src/compiler/builtin_symbol_table.cpp (right):
https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol_table.cpp#newcode12
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_table.h
File src/compiler/builtin_symbol_table.h (right):
https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol_table.h#newcode9
src/compiler/builtin_symbol_table.h:9: #include "GLSLANG/ShaderLang.h"
Is there a reason for including this file here?
Message from unknown
2013-06-05T23:25:33+00:00Alan Leung Chromiumurn:md5:4da24baefad77603337f24a4bc3d49fa
Message from acleung@chromium.org
2013-06-05T23:26:07+00:00Alan Leung Chromiumurn:md5:30eebf70a2d6c4d79f0df7ad394d7de0
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#oldcode503
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_table.cpp
File src/compiler/builtin_symbol_table.cpp (right):
https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol_table.cpp#newcode12
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_table.h
File src/compiler/builtin_symbol_table.h (right):
https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol_table.h#newcode9
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.
Message from alokp@chromium.org
2013-06-05T23:55:42+00:00Alok Priyadarshiurn:md5:2e24d6367ceb0ec39f8d7eba091f998e
https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol_table.cpp
File src/compiler/builtin_symbol_table.cpp (right):
https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol_table.cpp#newcode12
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.
Message from unknown
2013-06-06T22:20:21+00:00Alan Leung Chromiumurn:md5:770339a5b2705526d6f5ae1659bbc5f9
Message from acleung@chromium.org
2013-06-06T22:21:11+00:00Alan Leung Chromiumurn:md5:549878da310a6d6a2ac8a4392419190b
On 2013/06/05 23:55:42, Alok Priyadarshi wrote:
> https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol_table.cpp
> File src/compiler/builtin_symbol_table.cpp (right):
>
> https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol_table.cpp#newcode12
> 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.
Message from acleung@chromium.org
2013-06-06T22:21:41+00:00Alan Leung Chromiumurn:md5:25741b6ae7c8cea2b5d208076c281c09
Forgot to send the comments as well.
https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol_table.cpp
File src/compiler/builtin_symbol_table.cpp (right):
https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol_table.cpp#newcode12
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.
Message from alokp@chromium.org
2013-06-06T23:26:45+00:00Alok Priyadarshiurn:md5:50995af71881e559f3d9cfb46b32cdec
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_table.cpp
> File src/compiler/builtin_symbol_table.cpp (right):
>
> https://codereview.appspot.com/9436044/diff/47001/src/compiler/builtin_symbol_table.cpp#newcode12
> 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
Message from unknown
2013-06-07T00:00:13+00:00Alan Leung Chromiumurn:md5:7854ea7bbd962cb5c0d2a7aa240375e0
Message from unknown
2013-06-07T00:17:49+00:00Alan Leung Chromiumurn:md5:1018065bd3a897bfceb40b0251e6ed55
Message from acleung@chromium.org
2013-06-07T00:26:09+00:00Alan Leung Chromiumurn:md5:bc62b214a220c4ad69c6969a6d2ced1b
Committed patchset #14 manually as r2425 (presubmit successful).