Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
628453002:
Create a single command buffer for GrInOrderDrawBuffer (Closed)
|
Created:
6 years, 2 months ago by Chris Dalton Modified:
6 years, 2 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Project:
skia Visibility:
Public. |
DescriptionAdds a GrTRecorder class that GrInOrderDrawBuffer uses to allocate
all its commands interleaved in contiguous memory. GrTRecorder also
supports extra data associated with objects, so we can store arrays
inline without having to call malloc().
Committed: https://skia.googlesource.com/skia/+/360b6801cfd90485891d709e44cf395d527ba69e
Committed: https://skia.googlesource.com/skia/+/6819df36446f6fdcbd17d83a72a03de46e6d0d2d
Patch Set 1 #
Total comments: 20
Patch Set 2 : #
Total comments: 13
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Fix winodws build #Patch Set 7 : Fix issue caught by windows build -- Cmd should be noncopyable #Patch Set 8 : Fix meaningless windows warning #Patch Set 9 : Make copy ctor actually a copy ctor #Patch Set 10 : Remove 0-length arrays #Patch Set 11 : One more issue with msvc in the GrTRecorderTest.cpp #Patch Set 12 : Formatting #
Total comments: 2
Patch Set 13 : Single malloc for BemBlock #
Total comments: 2
Patch Set 14 : Comment #Patch Set 15 : #Patch Set 16 : #
Total comments: 1
Patch Set 17 : New windows warning #Patch Set 18 : Memleak #
Total comments: 2
Patch Set 19 : SkNoncopyable, formatting #Patch Set 20 : #
Messages
Total messages: 59 (15 generated)
cdalton@nvidia.com changed reviewers: + bsalomon@google.com
bsalomon@google.com changed reviewers: + mtklein@google.com, reed@google.com
Adding all the Mikes who have more experience with these types of patterns than I do. https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h File src/gpu/GrCmdBuffer.h (right): https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode26 src/gpu/GrCmdBuffer.h:26: template<typename TAlign = long double> class GrCmdBuffer : SkNoncopyable { Do long doubles require alignment? Wondering if void* is always sufficient. https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode71 src/gpu/GrCmdBuffer.h:71: TCmd& push_back() { return this->push_back_size<TCmd>(sizeof(TCmd)); } Instead of all these could we use something like a new override and a macro that does in-place new? (e.g. SkNEW_APPEND_TO_TARRAY, SkNEW_INSERT_AT_LLIST_HEAD, ...) https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode83 src/gpu/GrCmdBuffer.h:83: * @param sizeInBytes The amount of memory to allocate for the command. This may be Wonder whether we should only take the extra payload size and not the total size. The fData[0] thing feels overly tricky to me. What about virtual void execute(GrDrawTarget*, void* payload) ? https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcod... src/gpu/GrCmdBuffer.h:131: uint8_t fType; Is the type really just just used to hang the debug marker bit? If so could we do something like have the cmds in GrIODB that may need to have a dbg marker inherit from a base cmd type that has a bool? I ask because it feels like if we're paying for virtuality then it would be nicer to go all the way and not have to know the type being stored.
https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h File src/gpu/GrCmdBuffer.h (right): https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode26 src/gpu/GrCmdBuffer.h:26: template<typename TAlign = long double> class GrCmdBuffer : SkNoncopyable { On 2014/10/03 15:04:45, bsalomon wrote: > Do long doubles require alignment? Wondering if void* is always sufficient. According to this, long double gets aligned at 16 bytes: http://www.codesynthesis.com/~boris/blog/2009/04/06/cxx-data-alignment-portab... Whether that is necessary is questionable, especially since it just boils down to performance and the likelihood of ever storing a long double in here is basically nil. I think I'd be happy to remove that template parameter and align everything at void* if that's the route we decide to take. https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode71 src/gpu/GrCmdBuffer.h:71: TCmd& push_back() { return this->push_back_size<TCmd>(sizeof(TCmd)); } On 2014/10/03 15:04:45, bsalomon wrote: > Instead of all these could we use something like a new override and a macro that > does in-place new? > > (e.g. SkNEW_APPEND_TO_TARRAY, SkNEW_INSERT_AT_LLIST_HEAD, ...) > I did consider that approach. The only problem is that (without getting really creative) push_back_raw() and init_back() then need to become public, and they can mess things up if used wrong. They're meant to only be used in a very specific order: reserve data, in-place new, initialize Cmd base class. So for that reason I opted to do it like SkSmallAllocator. Has Skia adopted C++11? If so we could use variadic temples... https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode83 src/gpu/GrCmdBuffer.h:83: * @param sizeInBytes The amount of memory to allocate for the command. This may be On 2014/10/03 15:04:45, bsalomon wrote: > Wonder whether we should only take the extra payload size and not the total > size. > > The fData[0] thing feels overly tricky to me. What about > > virtual void execute(GrDrawTarget*, void* payload) ? The Cmd base class could also maybe know where to find its payload. https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcod... src/gpu/GrCmdBuffer.h:131: uint8_t fType; On 2014/10/03 15:04:45, bsalomon wrote: > Is the type really just just used to hang the debug marker bit? If so could we > do something like have the cmds in GrIODB that may need to have a dbg marker > inherit from a base cmd type that has a bool? > > I ask because it feels like if we're paying for virtuality then it would be > nicer to go all the way and not have to know the type being stored. At least for now, concatInstancedDraw() also checks the type: if (kDraw_Cmd != strip_trace_bit(fCmdBuffer.back().type())) { return 0; }
https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h File src/gpu/GrCmdBuffer.h (right): https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode26 src/gpu/GrCmdBuffer.h:26: template<typename TAlign = long double> class GrCmdBuffer : SkNoncopyable { On 2014/10/03 16:16:24, Chris Dalton wrote: > On 2014/10/03 15:04:45, bsalomon wrote: > > Do long doubles require alignment? Wondering if void* is always sufficient. > > According to this, long double gets aligned at 16 bytes: > > http://www.codesynthesis.com/~boris/blog/2009/04/06/cxx-data-alignment-portab... > > Whether that is necessary is questionable, especially since it just boils down > to performance and the likelihood of ever storing a long double in here is > basically nil. I think I'd be happy to remove that template parameter and align > everything at void* if that's the route we decide to take. Interesting. Maybe just have no default? https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode71 src/gpu/GrCmdBuffer.h:71: TCmd& push_back() { return this->push_back_size<TCmd>(sizeof(TCmd)); } On 2014/10/03 16:16:24, Chris Dalton wrote: > On 2014/10/03 15:04:45, bsalomon wrote: > > Instead of all these could we use something like a new override and a macro > that > > does in-place new? > > > > (e.g. SkNEW_APPEND_TO_TARRAY, SkNEW_INSERT_AT_LLIST_HEAD, ...) > > > > I did consider that approach. The only problem is that (without getting really > creative) push_back_raw() and init_back() then need to become public, and they > can mess things up if used wrong. They're meant to only be used in a very > specific order: reserve data, in-place new, initialize Cmd base class. So for > that reason I opted to do it like SkSmallAllocator. In the other instances where we've done this we just make the op new overload be friends with the container class to access a private raw push. > > Has Skia adopted C++11? If so we could use variadic temples... Not yet, hopefully soon. https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode83 src/gpu/GrCmdBuffer.h:83: * @param sizeInBytes The amount of memory to allocate for the command. This may be On 2014/10/03 16:16:24, Chris Dalton wrote: > On 2014/10/03 15:04:45, bsalomon wrote: > > Wonder whether we should only take the extra payload size and not the total > > size. > > > > The fData[0] thing feels overly tricky to me. What about > > > > virtual void execute(GrDrawTarget*, void* payload) ? > > The Cmd base class could also maybe know where to find its payload. Sure but then we need to store ptr. If it's a param then it just needs to exist at execute time. https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcod... src/gpu/GrCmdBuffer.h:131: uint8_t fType; On 2014/10/03 16:16:24, Chris Dalton wrote: > On 2014/10/03 15:04:45, bsalomon wrote: > > Is the type really just just used to hang the debug marker bit? If so could > we > > do something like have the cmds in GrIODB that may need to have a dbg marker > > inherit from a base cmd type that has a bool? > > > > I ask because it feels like if we're paying for virtuality then it would be > > nicer to go all the way and not have to know the type being stored. > > At least for now, concatInstancedDraw() also checks the type: > > if (kDraw_Cmd != strip_trace_bit(fCmdBuffer.back().type())) { > return 0; > } The GrIODB could just have a bool, fLastCmdWasDraw. Or the GrIODB's cmd structs could share a base class and that could have fType. It seems like having fType here is baking GrIODB's particulars into a reusable component.
https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h File src/gpu/GrCmdBuffer.h (right): https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode26 src/gpu/GrCmdBuffer.h:26: template<typename TAlign = long double> class GrCmdBuffer : SkNoncopyable { On 2014/10/03 18:43:03, bsalomon wrote: > On 2014/10/03 16:16:24, Chris Dalton wrote: > > On 2014/10/03 15:04:45, bsalomon wrote: > > > Do long doubles require alignment? Wondering if void* is always sufficient. > > > > According to this, long double gets aligned at 16 bytes: > > > > > http://www.codesynthesis.com/~boris/blog/2009/04/06/cxx-data-alignment-portab... > > > > Whether that is necessary is questionable, especially since it just boils down > > to performance and the likelihood of ever storing a long double in here is > > basically nil. I think I'd be happy to remove that template parameter and > align > > everything at void* if that's the route we decide to take. > > Interesting. Maybe just have no default? Sure https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode71 src/gpu/GrCmdBuffer.h:71: TCmd& push_back() { return this->push_back_size<TCmd>(sizeof(TCmd)); } On 2014/10/03 18:43:03, bsalomon wrote: > On 2014/10/03 16:16:24, Chris Dalton wrote: > > On 2014/10/03 15:04:45, bsalomon wrote: > > > Instead of all these could we use something like a new override and a macro > > that > > > does in-place new? > > > > > > (e.g. SkNEW_APPEND_TO_TARRAY, SkNEW_INSERT_AT_LLIST_HEAD, ...) > > > > > > > I did consider that approach. The only problem is that (without getting really > > creative) push_back_raw() and init_back() then need to become public, and they > > can mess things up if used wrong. They're meant to only be used in a very > > specific order: reserve data, in-place new, initialize Cmd base class. So for > > that reason I opted to do it like SkSmallAllocator. > > In the other instances where we've done this we just make the op new overload be > friends with the container class to access a private raw push. > > > > > > Has Skia adopted C++11? If so we could use variadic temples... > > Not yet, hopefully soon. Ok, this will work now. (The issue used to be that there was data to initialize in the Cmd class after the constructor (and therefore after operator new returned), but now that we're taking out fType, this isn't a problem anymore.) https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode83 src/gpu/GrCmdBuffer.h:83: * @param sizeInBytes The amount of memory to allocate for the command. This may be On 2014/10/03 18:43:03, bsalomon wrote: > On 2014/10/03 16:16:24, Chris Dalton wrote: > > On 2014/10/03 15:04:45, bsalomon wrote: > > > Wonder whether we should only take the extra payload size and not the total > > > size. > > > > > > The fData[0] thing feels overly tricky to me. What about > > > > > > virtual void execute(GrDrawTarget*, void* payload) ? > > > > The Cmd base class could also maybe know where to find its payload. > > Sure but then we need to store ptr. If it's a param then it just needs to exist > at execute time. So at execute time, the client code doesn't know where the payload is stored, and neither does the command buffer. (It would be right after the end of the Cmd subclass, but the full size of that subclass is unknown, and the Cmd* address might not be the same as its subclass's address.) It was my understanding that a zero-length array is standard for a struct that is the header of a variable-length object, but if we don't go that route, it seems we would either need to store a ptr/offset or else the subclass would have to find it with pointer arithmetic during execute(). https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcod... src/gpu/GrCmdBuffer.h:131: uint8_t fType; On 2014/10/03 18:43:03, bsalomon wrote: > On 2014/10/03 16:16:24, Chris Dalton wrote: > > On 2014/10/03 15:04:45, bsalomon wrote: > > > Is the type really just just used to hang the debug marker bit? If so could > > we > > > do something like have the cmds in GrIODB that may need to have a dbg marker > > > inherit from a base cmd type that has a bool? > > > > > > I ask because it feels like if we're paying for virtuality then it would be > > > nicer to go all the way and not have to know the type being stored. > > > > At least for now, concatInstancedDraw() also checks the type: > > > > if (kDraw_Cmd != strip_trace_bit(fCmdBuffer.back().type())) { > > return 0; > > } > > The GrIODB could just have a bool, fLastCmdWasDraw. Or the GrIODB's cmd structs > could share a base class and that could have fType. It seems like having fType > here is baking GrIODB's particulars into a reusable component. > Oh, fLastCmdWasDraw, I like that
GrTBaseList was the best name I could come up with. I'm open to trying to find a better name.
https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h File src/gpu/GrCmdBuffer.h (right): https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode26 src/gpu/GrCmdBuffer.h:26: template<typename TAlign = long double> class GrCmdBuffer : SkNoncopyable { On 2014/10/03 19:38:40, Chris Dalton wrote: > On 2014/10/03 18:43:03, bsalomon wrote: > > On 2014/10/03 16:16:24, Chris Dalton wrote: > > > On 2014/10/03 15:04:45, bsalomon wrote: > > > > Do long doubles require alignment? Wondering if void* is always > sufficient. > > > > > > According to this, long double gets aligned at 16 bytes: > > > > > > > > > http://www.codesynthesis.com/~boris/blog/2009/04/06/cxx-data-alignment-portab... > > > > > > Whether that is necessary is questionable, especially since it just boils > down > > > to performance and the likelihood of ever storing a long double in here is > > > basically nil. I think I'd be happy to remove that template parameter and > > align > > > everything at void* if that's the route we decide to take. > > > > Interesting. Maybe just have no default? > > Sure Done. https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode71 src/gpu/GrCmdBuffer.h:71: TCmd& push_back() { return this->push_back_size<TCmd>(sizeof(TCmd)); } On 2014/10/03 19:38:40, Chris Dalton wrote: > On 2014/10/03 18:43:03, bsalomon wrote: > > On 2014/10/03 16:16:24, Chris Dalton wrote: > > > On 2014/10/03 15:04:45, bsalomon wrote: > > > > Instead of all these could we use something like a new override and a > macro > > > that > > > > does in-place new? > > > > > > > > (e.g. SkNEW_APPEND_TO_TARRAY, SkNEW_INSERT_AT_LLIST_HEAD, ...) > > > > > > > > > > I did consider that approach. The only problem is that (without getting > really > > > creative) push_back_raw() and init_back() then need to become public, and > they > > > can mess things up if used wrong. They're meant to only be used in a very > > > specific order: reserve data, in-place new, initialize Cmd base class. So > for > > > that reason I opted to do it like SkSmallAllocator. > > > > In the other instances where we've done this we just make the op new overload > be > > friends with the container class to access a private raw push. > > > > > > > > > > Has Skia adopted C++11? If so we could use variadic temples... > > > > Not yet, hopefully soon. > > Ok, this will work now. (The issue used to be that there was data to initialize > in the Cmd class after the constructor (and therefore after operator new > returned), but now that we're taking out fType, this isn't a problem anymore.) Done. https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcode83 src/gpu/GrCmdBuffer.h:83: * @param sizeInBytes The amount of memory to allocate for the command. This may be On 2014/10/03 19:38:40, Chris Dalton wrote: > On 2014/10/03 18:43:03, bsalomon wrote: > > On 2014/10/03 16:16:24, Chris Dalton wrote: > > > On 2014/10/03 15:04:45, bsalomon wrote: > > > > Wonder whether we should only take the extra payload size and not the > total > > > > size. > > > > > > > > The fData[0] thing feels overly tricky to me. What about > > > > > > > > virtual void execute(GrDrawTarget*, void* payload) ? > > > > > > The Cmd base class could also maybe know where to find its payload. > > > > Sure but then we need to store ptr. If it's a param then it just needs to > exist > > at execute time. > > So at execute time, the client code doesn't know where the payload is stored, > and neither does the command buffer. (It would be right after the end of the Cmd > subclass, but the full size of that subclass is unknown, and the Cmd* address > might not be the same as its subclass's address.) > > It was my understanding that a zero-length array is standard for a struct that > is the header of a variable-length object, but if we don't go that route, it > seems we would either need to store a ptr/offset or else the subclass would have > to find it with pointer arithmetic during execute(). Left as-is for now, to resemble the way this would be handled with malloc: malloc(sizeof(T) + payload); https://codereview.chromium.org/628453002/diff/1/src/gpu/GrCmdBuffer.h#newcod... src/gpu/GrCmdBuffer.h:131: uint8_t fType; On 2014/10/03 19:38:40, Chris Dalton wrote: > On 2014/10/03 18:43:03, bsalomon wrote: > > On 2014/10/03 16:16:24, Chris Dalton wrote: > > > On 2014/10/03 15:04:45, bsalomon wrote: > > > > Is the type really just just used to hang the debug marker bit? If so > could > > > we > > > > do something like have the cmds in GrIODB that may need to have a dbg > marker > > > > inherit from a base cmd type that has a bool? > > > > > > > > I ask because it feels like if we're paying for virtuality then it would > be > > > > nicer to go all the way and not have to know the type being stored. > > > > > > At least for now, concatInstancedDraw() also checks the type: > > > > > > if (kDraw_Cmd != strip_trace_bit(fCmdBuffer.back().type())) { > > > return 0; > > > } > > > > The GrIODB could just have a bool, fLastCmdWasDraw. Or the GrIODB's cmd > structs > > could share a base class and that could have fType. It seems like having fType > > here is baking GrIODB's particulars into a reusable component. > > > > Oh, fLastCmdWasDraw, I like that I ended up leaving fType in the base Cmd class. It's used to track trace markers as well as make asserts during flush. I'm still open to removing it.
I'm OK with keeping fCmd now that it's on the GrIODB's base class and not a requirement for the container class. https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.cpp (right): https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:399: int size = sizeof(DrawPaths) + indicesSize + transformsSize; Maybe have the macro take the extra size? I can't imagine a scenario where someone would want anything less than sizeof(T). GrNEW_APPEND_WITH_DATA_TO_TBASELIST(list, dataSize, T, (args))? https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrTBaseList.h File src/gpu/GrTBaseList.h (right): https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrTBaseList.h#ne... src/gpu/GrTBaseList.h:21: * List of objects with a common base type and permanent memory addresses. This thing seems complicated enough, and isolated enough, to have some unit tests. https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrTBaseList.h#ne... src/gpu/GrTBaseList.h:24: * objects can be created without excessive calls to malloc(). Comment about macros to push back? Casual user of this might not read to the end of the file. https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrTBaseList.h#ne... src/gpu/GrTBaseList.h:33: * on an obscure compiler) may not be compatible. This is verified at Can we say "runtime asserted in debug builds" rather than "verified at runtime"? https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrTBaseList.h#ne... src/gpu/GrTBaseList.h:40: template<typename TBase, typename TAlign> class GrTBaseList : SkNoncopyable { I'm having trouble coming up with a better name https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrTBaseList.h#ne... src/gpu/GrTBaseList.h:45: * Create a contiguous list Can we not say contiguous? That seems like an impl detail (and not strictly true) https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrTBaseList.h#ne... src/gpu/GrTBaseList.h:111: void* operator new(size_t expectedClassSize, I think you'll need an operator delete. We don't compile with exceptions enabled but clients of skia do and I think you get an error if op new is not matched with op delete.
https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.cpp (right): https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:399: int size = sizeof(DrawPaths) + indicesSize + transformsSize; On 2014/10/08 19:51:44, bsalomon wrote: > Maybe have the macro take the extra size? I can't imagine a scenario where > someone would want anything less than sizeof(T). > > GrNEW_APPEND_WITH_DATA_TO_TBASELIST(list, dataSize, T, (args))? Done. https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrTBaseList.h File src/gpu/GrTBaseList.h (right): https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrTBaseList.h#ne... src/gpu/GrTBaseList.h:21: * List of objects with a common base type and permanent memory addresses. On 2014/10/08 19:51:44, bsalomon wrote: > This thing seems complicated enough, and isolated enough, to have some unit > tests. Done. https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrTBaseList.h#ne... src/gpu/GrTBaseList.h:24: * objects can be created without excessive calls to malloc(). On 2014/10/08 19:51:44, bsalomon wrote: > Comment about macros to push back? Casual user of this might not read to the end > of the file. Done. https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrTBaseList.h#ne... src/gpu/GrTBaseList.h:33: * on an obscure compiler) may not be compatible. This is verified at On 2014/10/08 19:51:45, bsalomon wrote: > Can we say "runtime asserted in debug builds" rather than "verified at runtime"? Done. https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrTBaseList.h#ne... src/gpu/GrTBaseList.h:45: * Create a contiguous list On 2014/10/08 19:51:44, bsalomon wrote: > Can we not say contiguous? That seems like an impl detail (and not strictly > true) Oops, I missed that one from before. Done. https://codereview.chromium.org/628453002/diff/20001/src/gpu/GrTBaseList.h#ne... src/gpu/GrTBaseList.h:111: void* operator new(size_t expectedClassSize, On 2014/10/08 19:51:44, bsalomon wrote: > I think you'll need an operator delete. We don't compile with exceptions enabled > but clients of skia do and I think you get an error if op new is not matched > with op delete. Done.
I realized I'm not crazy about the word "List" because this doesn't allow random insertions/deletions. If it had a pop we could call it a GrTBaseStack, but then you'd expect the iter to go back to front. I'm kind of at a loss. GrTBaseRecorder? https://codereview.chromium.org/628453002/diff/40001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.cpp (right): https://codereview.chromium.org/628453002/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:407: if (NULL != dstCopy) { we recently dropped this style, can be "if (dstCopy)" https://codereview.chromium.org/628453002/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:794: #include <stdio.h> Is this required? I didn't notice any usage below.
Renamed to GrTRecorder
can stdio go away?
On 2014/10/09 18:37:53, bsalomon wrote: > can stdio go away? Oops sorry, didn't catch your most recent comments. It'
I'm not sure what key combination I accidentally pressed but it published my message mid-sentence :) Stdio is gone, and I double checked for any other dangling debug code. https://codereview.chromium.org/628453002/diff/40001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.cpp (right): https://codereview.chromium.org/628453002/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:407: if (NULL != dstCopy) { On 2014/10/09 01:31:14, bsalomon wrote: > we recently dropped this style, can be "if (dstCopy)" Done. https://codereview.chromium.org/628453002/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:794: #include <stdio.h> On 2014/10/09 01:31:14, bsalomon wrote: > Is this required? I didn't notice any usage below. Done.
lgtm
The CQ bit was checked by cdalton@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/628453002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)
The CQ bit was checked by cdalton@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/628453002/310001
The CQ bit was checked by cdalton@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/628453002/400001
The CQ bit was checked by cdalton@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/628453002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)
The CQ bit was checked by cdalton@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/628453002/590001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)
Removed 0-length arrays and instead added logic for GrTRecorder to handle extra data. This change is fairly big, so it may warrant one more quick review.
https://codereview.chromium.org/628453002/diff/850001/src/gpu/GrTRecorder.h File src/gpu/GrTRecorder.h (right): https://codereview.chromium.org/628453002/diff/850001/src/gpu/GrTRecorder.h#n... src/gpu/GrTRecorder.h:104: MemBlock(int length) : fLength(length), fBack(0), fBuffer(fLength) {} I didn't pay that much attention to MemBlock before.. Now I've got a question: Why use a separate allocation for Memblock and it's storage rather than one allocation where the first part is a memblock struct?
https://codereview.chromium.org/628453002/diff/850001/src/gpu/GrTRecorder.h File src/gpu/GrTRecorder.h (right): https://codereview.chromium.org/628453002/diff/850001/src/gpu/GrTRecorder.h#n... src/gpu/GrTRecorder.h:104: MemBlock(int length) : fLength(length), fBack(0), fBuffer(fLength) {} On 2014/10/11 11:32:49, bsalomon wrote: > I didn't pay that much attention to MemBlock before.. Now I've got a question: > Why use a separate allocation for Memblock and it's storage rather than one > allocation where the first part is a memblock struct? It was just a quick code complexity vs perf impact decision. I've changed it to only do one malloc now and I quite like it.
lgtm https://codereview.chromium.org/628453002/diff/990001/src/gpu/GrTRecorder.h File src/gpu/GrTRecorder.h (right): https://codereview.chromium.org/628453002/diff/990001/src/gpu/GrTRecorder.h#n... src/gpu/GrTRecorder.h:207: iter->~TBase(); maybe the class comment should document the destructor order (queue not stack order)?
https://codereview.chromium.org/628453002/diff/990001/src/gpu/GrTRecorder.h File src/gpu/GrTRecorder.h (right): https://codereview.chromium.org/628453002/diff/990001/src/gpu/GrTRecorder.h#n... src/gpu/GrTRecorder.h:207: iter->~TBase(); On 2014/10/13 18:33:24, bsalomon wrote: > maybe the class comment should document the destructor order (queue not stack > order)? Done.
The CQ bit was checked by cdalton@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/628453002/1050001
Message was sent while issue was closed.
Committed patchset #14 (id:1050001) as 47c844aaba81e5a29c773b660e1d6062c766d253
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:1050001) has been created in https://codereview.chromium.org/652843002/ by mtklein@google.com. The reason for reverting is: New test failing on Android: http://build.chromium.org/p/client.skia.android/builders/Test-Android-Nexus7-....
Message was sent while issue was closed.
https://codereview.chromium.org/628453002/diff/1210001/tests/GrTRecorderTest.cpp File tests/GrTRecorderTest.cpp (right): https://codereview.chromium.org/628453002/diff/1210001/tests/GrTRecorderTest.... tests/GrTRecorderTest.cpp:154: data[i] = ValueAt(i); I still don't understand this fully, but in a release build, the following code consistently leaves data[i] unchanged: data[i] = 123456789 + 987654321 * i; I verified this by calling memset immediately before the loop. Oddly enough, these other operations succeed in updating data[i]: data[i] = i; data[i] = 123456789 + 987654321 * (uint64_t)i; data[i] = ValueAt(i); I am unsure if this due to invalid type punning or a compiler bug. But I did experiment with explicit char* conversions and it didn't fix the issue, and we aren't getting any warnings about invalid invalid type punning either.
Message was sent while issue was closed.
On 2014/10/13 23:17:53, Chris Dalton wrote: > https://codereview.chromium.org/628453002/diff/1210001/tests/GrTRecorderTest.cpp > File tests/GrTRecorderTest.cpp (right): > > https://codereview.chromium.org/628453002/diff/1210001/tests/GrTRecorderTest.... > tests/GrTRecorderTest.cpp:154: data[i] = ValueAt(i); > I still don't understand this fully, but in a release build, the following code > consistently leaves data[i] unchanged: > > data[i] = 123456789 + 987654321 * i; > > I verified this by calling memset immediately before the loop. > > Oddly enough, these other operations succeed in updating data[i]: > > data[i] = i; > data[i] = 123456789 + 987654321 * (uint64_t)i; > data[i] = ValueAt(i); > > I am unsure if this due to invalid type punning or a compiler bug. But I did > experiment with explicit char* conversions and it didn't fix the issue, and we > aren't getting any warnings about invalid invalid type punning either. Strange, if the new code works feel free to check the cq.
The CQ bit was checked by cdalton@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/628453002/1210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile (http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)
The CQ bit was checked by cdalton@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/628453002/1240001
Message was sent while issue was closed.
Committed patchset #17 (id:1240001) as 360b6801cfd90485891d709e44cf395d527ba69e
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:1240001) has been created in https://codereview.chromium.org/654863003/ by mtklein@google.com. The reason for reverting is: Leaking memory: http://build.chromium.org/p/client.skia/builders/Test-Ubuntu12-ShuttleA-GTX55....
Message was sent while issue was closed.
And the neverending patch continues .. Sorry, I made a rookie mistake and introduced a leak last minute when we switched to a single alloc for the MemBlock struct.
On 2014/10/14 22:31:57, Chris Dalton wrote: > And the neverending patch continues .. > > Sorry, I made a rookie mistake and introduced a leak last minute when we > switched to a single alloc for the MemBlock struct. No worries. This is why we have so many bots. :)
https://codereview.chromium.org/628453002/diff/1260001/src/gpu/GrTRecorder.h File src/gpu/GrTRecorder.h (right): https://codereview.chromium.org/628453002/diff/1260001/src/gpu/GrTRecorder.h#... src/gpu/GrTRecorder.h:127: TAlign& operator [](int i) { can you group this with the other methods? otherwise lgtm. Maybe inherit from ::SkNoncopyable?
https://codereview.chromium.org/628453002/diff/1260001/src/gpu/GrTRecorder.h File src/gpu/GrTRecorder.h (right): https://codereview.chromium.org/628453002/diff/1260001/src/gpu/GrTRecorder.h#... src/gpu/GrTRecorder.h:127: TAlign& operator [](int i) { On 2014/10/15 18:38:06, bsalomon wrote: > can you group this with the other methods? otherwise lgtm. > > Maybe inherit from ::SkNoncopyable? Done.
The CQ bit was checked by cdalton@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/628453002/1300001
Message was sent while issue was closed.
Committed patchset #20 (id:1300001) as 6819df36446f6fdcbd17d83a72a03de46e6d0d2d |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
