Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
155513012:
[WIP] Add Context to SkDrawLooper. (Closed)
|
Created:
6 years, 10 months ago by Dominik Grewe Modified:
6 years, 9 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
Description[WIP] Add Context to SkDrawLooper.
SkDrawLooper carries some state during draws. This CL extracts this state into
a separate class Context, which is then passed by the users of SkDrawLooper
into the appropriate methods.
This is a step towards making SkDrawLooper immutable.
BUG=skia:2141
Committed: http://code.google.com/p/skia/source/detail?r=13760
Patch Set 1 #Patch Set 2 : Make methods const. #
Total comments: 8
Patch Set 3 : Move draw logic into context. #Patch Set 4 : static allocation of DrawContext; update rest of code. #
Total comments: 7
Patch Set 5 : Move size constant to cpp files; add SK_PLACEMENT_SAFE_NEW directives #
Total comments: 2
Patch Set 6 : Add comments. #
Total comments: 8
Patch Set 7 : Implement SK_PLACEMENT_DELETE #Patch Set 8 : Use SkSmallAllocator #
Total comments: 6
Patch Set 9 : Make SkSmallAllocator private again. #
Total comments: 10
Patch Set 10 : comments #
Messages
Total messages: 40 (0 generated)
Some rough ideas/questions: 1. Can we allow effects with "small" state to not have to dynamically allocate it? e.g. State* effect->setup(..., storage, storage_size) Where storage is memory externally provided. If the effect's state can fit in storage_size, it uses that, else it calls new. 2. As much syntactic sugar as anything, what if the doing-it part of the API was moved to the State class itself. e.g. State* state = effect->setup(..., storage, storage_size); state->next(canvas); state->next(canvas); state->next(canvas); ...
On 2014/02/06 17:01:10, reed1 wrote: > Some rough ideas/questions: > > 1. Can we allow effects with "small" state to not have to dynamically allocate > it? > > e.g. > > State* effect->setup(..., storage, storage_size) > > Where storage is memory externally provided. If the effect's state can fit in > storage_size, it uses that, else it calls new. Yeah, I think that would be possible in principle. Where would the storage come from, though? Are there any similar examples in Skia that I could look at? > > 2. As much syntactic sugar as anything, what if the doing-it part of the API was > moved to the State class itself. > > e.g. > > State* state = effect->setup(..., storage, storage_size); > state->next(canvas); > state->next(canvas); > state->next(canvas); > ... It would certainly be possible. Then the effect itself would just hold the data and the state would have all the logic. I'll give it a try.
We use this external storage trick for allocating blitters, which never live longer than a stack-frame, so we put their storage on the stack. uint32_t storage[MAX]; call(storage, sizeof(storage));
Ignore my comments about SkNEW/SkDELETE once you use the stack allocation trick. Looks like it's on the right track to me. https://codereview.chromium.org/155513012/diff/30001/include/core/SkDrawLooper.h File include/core/SkDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/30001/include/core/SkDrawLoope... include/core/SkDrawLooper.h:59: bool next(SkCanvas* canvas, SkPaint* paint, DrawContext* context) const; It's a little weird that the parameter that's passed in may be deleted, but it may be the right way to do it. Could you add a comment here explaining that. (Assuming it is still dynamically allocated. Also, I know there's a comment above DrawContext, but I think it should be here as well.) https://codereview.chromium.org/155513012/diff/30001/include/core/SkDrawLoope... include/core/SkDrawLooper.h:82: virtual bool next_internal( I think our typical naming convention would be onNext(). https://codereview.chromium.org/155513012/diff/30001/include/effects/SkBlurDr... File include/effects/SkBlurDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/30001/include/effects/SkBlurDr... include/effects/SkBlurDrawLooper.h:47: virtual SkDrawLooper::DrawContext* init(SkCanvas*) const; SK_OVERRIDE https://codereview.chromium.org/155513012/diff/30001/include/effects/SkBlurDr... include/effects/SkBlurDrawLooper.h:56: virtual bool next_internal( SK_OVERRIDE https://codereview.chromium.org/155513012/diff/30001/src/core/SkDrawLooper.cpp File src/core/SkDrawLooper.cpp (right): https://codereview.chromium.org/155513012/diff/30001/src/core/SkDrawLooper.cp... src/core/SkDrawLooper.cpp:16: bool ret = next_internal(canvas, paint, context); this->next_internal (or onNext) https://codereview.chromium.org/155513012/diff/30001/src/core/SkDrawLooper.cp... src/core/SkDrawLooper.cpp:18: delete context; SkDELETE https://codereview.chromium.org/155513012/diff/30001/src/effects/SkBlurDrawLo... File src/effects/SkBlurDrawLooper.cpp (right): https://codereview.chromium.org/155513012/diff/30001/src/effects/SkBlurDrawLo... src/effects/SkBlurDrawLooper.cpp:93: return new SkBlurDrawLooperContext; SkNEW https://codereview.chromium.org/155513012/diff/30001/src/effects/SkLayerDrawL... File src/effects/SkLayerDrawLooper.cpp (right): https://codereview.chromium.org/155513012/diff/30001/src/effects/SkLayerDrawL... src/effects/SkLayerDrawLooper.cpp:80: SkLayerDrawLooperContext* context = new SkLayerDrawLooperContext(this); SkNEW_ARGS
On 2014/02/06 17:55:55, reed1 wrote: > We use this external storage trick for allocating blitters, which never live > longer than a stack-frame, so we put their storage on the stack. > > uint32_t storage[MAX]; > call(storage, sizeof(storage)); Okay, cool. I have a question about the case where the storage isn't large enough and you have allocate dynamically. Do you notify the caller so he can free the memory or do you set a bit in the object so it deletes itself?
Thanks for the feedback! I've updated the patch, so that the SkDrawContext now has all the logic, so we can simply call context->next(). I'll look into the static memory allocation tomorrow.
On 2014/02/06 18:23:08, Dominik Grewe wrote: > On 2014/02/06 17:55:55, reed1 wrote: > > We use this external storage trick for allocating blitters, which never live > > longer than a stack-frame, so we put their storage on the stack. > > > > uint32_t storage[MAX]; > > call(storage, sizeof(storage)); > > Okay, cool. I have a question about the case where the storage isn't large > enough and you have allocate dynamically. Do you notify the caller so he can > free the memory or do you set a bit in the object so it deletes itself? Not sure what the cleanest way to encapsulate this is, but I think at the heart of it is comparing the State* address to storage* address. If they're different, we should call delete state; else we should call state->~State();
I've added the static allocation. The user is responsible for calling a cleanup() function which ensures the correct destruction of the object (see comment below). I've also updated the rest of the code, so it now compiles and runs the tests successfully on my Linux machine. https://codereview.chromium.org/155513012/diff/190001/include/core/SkDrawLoop... File include/core/SkDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/190001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:15: #define kDrawLooperContextStorageLongCount (sizeof(void*) + sizeof(int)) It's not particularly nice to hardcode the size of the largest DrawContext subclass, but I'm not sure if we want to include the header for that class here (and the class is currently private). https://codereview.chromium.org/155513012/diff/190001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:62: void cleanup(void* storage); I've added this so the user is in charge of when to free the memory. This is required for canComputeFastBounds below, which may finish before next() returns false. https://codereview.chromium.org/155513012/diff/190001/src/effects/SkBlurDrawL... File src/effects/SkBlurDrawLooper.cpp (right): https://codereview.chromium.org/155513012/diff/190001/src/effects/SkBlurDrawL... src/effects/SkBlurDrawLooper.cpp:97: storageSize, (this)); This actually doesn't check if storageSize is large enough. It simply checks if it's not 0. I used it nonetheless because it's being used in similar situations in the code.
1. I realize internally we don't do a runtime check of storageSize against sizeof(subclass), but that is for internal classes that we control. This would be the first time we tried to make this avoid-new-trick public, and I think we need to be more robust, performing a runtime check. 2. I like the way this experiment is proceeding, partly because it is making us see difficulties in apply this technique to SkShader. Perhaps we can pause soon on this CL, and explore the issues that are raise in applying it to other effects (e.g. shader) before committing to it. https://codereview.chromium.org/155513012/diff/190001/include/core/SkDrawLoop... File include/core/SkDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/190001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:15: #define kDrawLooperContextStorageLongCount (sizeof(void*) + sizeof(int)) On 2014/02/07 11:57:15, Dominik Grewe wrote: > It's not particularly nice to hardcode the size of the largest DrawContext > subclass, but I'm not sure if we want to include the header for that class here > (and the class is currently private). 1. I think callers have to be able to survive subclasses that require more storage than we provided. 2. I agree that I *don't* want to make any public promises about the max-size of internal subclasses. I vote to remove this define. https://codereview.chromium.org/155513012/diff/190001/include/effects/SkLayer... File include/effects/SkLayerDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/190001/include/effects/SkLayer... include/effects/SkLayerDrawLooper.h:126: class LayerDrawLooperContext : public SkDrawLooper::DrawContext { Just for public brevity, can this definition be moved into the .cpp?
> 1. I realize internally we don't do a runtime check of storageSize against > sizeof(subclass), but that is for internal classes that we control. This would > be the first time we tried to make this avoid-new-trick public, and I think we > need to be more robust, performing a runtime check. I've add new directives (SK_PLACEMENT_SAFE_NEW) that does the runtime check. > 2. I like the way this experiment is proceeding, partly because it is making us > see difficulties in apply this technique to SkShader. Perhaps we can pause soon > on this CL, and explore the issues that are raise in applying it to other > effects (e.g. shader) before committing to it. Sounds good to me. I know Leon's working on SkShader but let me know if I can help in any way. As far as I've seen none of the other effects carry any state during draws. https://codereview.chromium.org/155513012/diff/190001/include/core/SkDrawLoop... File include/core/SkDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/190001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:15: #define kDrawLooperContextStorageLongCount (sizeof(void*) + sizeof(int)) On 2014/02/07 14:46:02, reed1 wrote: > On 2014/02/07 11:57:15, Dominik Grewe wrote: > > It's not particularly nice to hardcode the size of the largest DrawContext > > subclass, but I'm not sure if we want to include the header for that class > here > > (and the class is currently private). > > 1. I think callers have to be able to survive subclasses that require more > storage than we provided. > 2. I agree that I *don't* want to make any public promises about the max-size of > internal subclasses. > > I vote to remove this define. I'll move it into the cpp files instead. https://codereview.chromium.org/155513012/diff/190001/include/effects/SkLayer... File include/effects/SkLayerDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/190001/include/effects/SkLayer... include/effects/SkLayerDrawLooper.h:126: class LayerDrawLooperContext : public SkDrawLooper::DrawContext { On 2014/02/07 14:46:02, reed1 wrote: > Just for public brevity, can this definition be moved into the .cpp? If we move this definition out of the class scope we can't use private structs such as 'Rec' any more.
https://codereview.chromium.org/155513012/diff/270001/include/core/SkDrawLoop... File include/core/SkDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/270001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:60: void cleanup(void* storage); Dox?
https://codereview.chromium.org/155513012/diff/270001/include/core/SkDrawLoop... File include/core/SkDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/270001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:60: void cleanup(void* storage); On 2014/02/07 18:27:40, reed1 wrote: > Dox? Done.
On 2014/02/07 16:59:16, Dominik Grewe wrote: > > 1. I realize internally we don't do a runtime check of storageSize against > > sizeof(subclass), but that is for internal classes that we control. This would > > be the first time we tried to make this avoid-new-trick public, and I think we > > need to be more robust, performing a runtime check. > > I've add new directives (SK_PLACEMENT_SAFE_NEW) that does the runtime check. > > > > 2. I like the way this experiment is proceeding, partly because it is making > us > > see difficulties in apply this technique to SkShader. Perhaps we can pause > soon > > on this CL, and explore the issues that are raise in applying it to other > > effects (e.g. shader) before committing to it. > > Sounds good to me. I know Leon's working on SkShader but let me know if I can > help in any way. As far as I've seen none of the other effects carry any state > during draws. I'll try to get something up soon for you to look at my work in progress. https://codereview.chromium.org/155513012/diff/400001/include/core/SkDrawLoop... File include/core/SkDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/400001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:34: * final call to next() will also delete the context. nit: The word delete is no longer necessarily accurate here. https://codereview.chromium.org/155513012/diff/400001/src/core/SkTemplatesPriv.h File src/core/SkTemplatesPriv.h (right): https://codereview.chromium.org/155513012/diff/400001/src/core/SkTemplatesPri... src/core/SkTemplatesPriv.h:25: result = SkNEW(classname) Yikes! This will result in a memory leak if the caller is not careful. Would it make sense to have some kind of SK_PLACEMENT_SAFE_DELETE that handles the delete the way you do your delete? I see this is already what we do on WIN32 for normal placement new (which I cannot imagine the caller accounting for). Does placement new not work on WIN32? (The history of SK_PLACEMENT_NEW on WIN32 is lost to time - it was pulled from Android in 2008.)
https://codereview.chromium.org/155513012/diff/400001/include/core/SkDrawLoop... File include/core/SkDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/400001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:34: * final call to next() will also delete the context. On 2014/02/11 15:51:29, scroggo wrote: > nit: The word delete is no longer necessarily accurate here. Good point. Actually the last call to next() doesn't do anything any more. We ask the user to explicitly call cleanup(), because we don't always finish the entire loop. I'll update the comment accordingly. https://codereview.chromium.org/155513012/diff/400001/src/core/SkTemplatesPriv.h File src/core/SkTemplatesPriv.h (right): https://codereview.chromium.org/155513012/diff/400001/src/core/SkTemplatesPri... src/core/SkTemplatesPriv.h:25: result = SkNEW(classname) On 2014/02/11 15:51:29, scroggo wrote: > Yikes! This will result in a memory leak if the caller is not careful. Would it > make sense to have some kind of SK_PLACEMENT_SAFE_DELETE that handles the delete > the way you do your delete? I guess the same is true for the normal placement method. No matter which one is used, you always have to be careful about freeing memory. Having SK_PLACEMENT_DELETE would make sense; I don't think we need a special SK_PLACEMENT_SAFE_DELETE but we could add it for "symmetry". > I see this is already what we do on WIN32 for normal placement new (which I > cannot imagine the caller accounting for). Does placement new not work on WIN32? > > (The history of SK_PLACEMENT_NEW on WIN32 is lost to time - it was pulled from > Android in 2008.) Tbh, I'm not sure why we need this special case for WIN32, I just assumed we do because that's what we do the normal placement functions. I can have a look if it's still needed.
https://codereview.chromium.org/155513012/diff/400001/src/core/SkTemplatesPriv.h File src/core/SkTemplatesPriv.h (right): https://codereview.chromium.org/155513012/diff/400001/src/core/SkTemplatesPri... src/core/SkTemplatesPriv.h:25: result = SkNEW(classname) On 2014/02/11 16:04:31, Dominik Grewe wrote: > On 2014/02/11 15:51:29, scroggo wrote: > > Yikes! This will result in a memory leak if the caller is not careful. Would > it > > make sense to have some kind of SK_PLACEMENT_SAFE_DELETE that handles the > delete > > the way you do your delete? > > I guess the same is true for the normal placement method. No matter which one is > used, you always have to be careful about freeing memory. Well, this one is actually safer than the normal placement method. When calling this, you (should) know that you have to special case the delete, but if someone called SK_PLACEMENT_NEW I would expect them to call ~T() instead of delete, which would be the appropriate place everywhere except WIN32, where we'll leak the memory. (Sorry to derail your CL a little bit - this problem wasn't introduced by you!) On another note, what about using SkAutoSMalloc (in SkTypes.h)? This class handles choosing whether to use malloc and free (although you then still have to do the SK_PLACEMENT_NEW yourself). > > Having SK_PLACEMENT_DELETE would make sense; I don't think we need a special > SK_PLACEMENT_SAFE_DELETE but we could add it for "symmetry". > > > > I see this is already what we do on WIN32 for normal placement new (which I > > cannot imagine the caller accounting for). Does placement new not work on > WIN32? > > > > (The history of SK_PLACEMENT_NEW on WIN32 is lost to time - it was pulled from > > Android in 2008.) > > Tbh, I'm not sure why we need this special case for WIN32, I just assumed we do > because that's what we do the normal placement functions. I can have a look if > it's still needed.
Hadn't taken a look at SkTemplatesPriv.h before, and didn't realize it was so bad. These comments may be something we need to do later. https://codereview.chromium.org/155513012/diff/400001/src/core/SkTemplatesPriv.h File src/core/SkTemplatesPriv.h (right): https://codereview.chromium.org/155513012/diff/400001/src/core/SkTemplatesPri... src/core/SkTemplatesPriv.h:17: #ifdef SK_BUILD_FOR_WIN32 This seems to be an anachronism. I see no indication that vc++ ever had any actual issues with placement new (and if it does I'm fairly sure there are other places in Skia which would break). Note that SkPostConfig.h already provides SkNEW_PLACEMENT which is used elsewhere on all platforms. I just disabled this (#if 0) and it builds fine on vs2012 (and I'm sure on all others). It looks like this is mostly holdover from the very early attempt at a Windows port since this file and content appear to come from before Windows was actually supported. https://codereview.chromium.org/155513012/diff/400001/src/core/SkTemplatesPri... src/core/SkTemplatesPriv.h:31: #define SK_PLACEMENT_NEW(result, classname, storage, storagesize) \ Every current user of this does the same dance where they have to do a check to see what result was and either delete or destroy. The name is also entirely misleading, as this obviously isn't interchangeable with SkNEW_PLACEMENT. In other words, the name is terrible and doesn't do what it says it does. Let's not perpetuate that. https://codereview.chromium.org/155513012/diff/400001/src/core/SkTemplatesPri... src/core/SkTemplatesPriv.h:53: #define SK_PLACEMENT_SAFE_NEW(result, classname, storage, storagesize) \ Why are we multiplying entities without necessity here? Why the new name? Why not just replace the content of the SK_PLACEMENT_NEW with these new bodies? Comparing against the constant sizeof or against 0 here seems a rather small difference in performance, if that's what we're worrying about.
I've added a SK_PLACEMENT_DELETE which makes sure that we correctly destroy the object. I'm happy to merge SK_PLACEMENT_SAFE_NEW into SK_PLACEMENT_NEW, but I leave the final decision on that to you guys :)
I tried to rebase this patch and use Leon's new SkSmallAllocator instead of a raw pointer. That would require for SkSmallAllocator to be publicly visible though, as we want users to pass it in to SkDrawLooper::init(). Is there a fundamental reason why SkSmallAllocator is not public? Or should I just stick to using a raw storage pointer? Btw, I guess we'll run into the same issue with SkShader.
On 2014/03/10 13:12:17, Dominik Grewe wrote: > I tried to rebase this patch and use Leon's new SkSmallAllocator instead of a > raw pointer. That would require for SkSmallAllocator to be publicly visible > though, as we want users to pass it in to SkDrawLooper::init(). Is there a > fundamental reason why SkSmallAllocator is not public? Or should I just stick to > using a raw storage pointer? The reason I didn't make it public was for a few reasons: - It is designed for a very specific purpose - Its API may continue to evolve - It's an optimization rather than providing a feature that allows you to use graphics in a program - I didn't see a need for it to be public (which doesn't mean there isn't one or couldn't be one) > > Btw, I guess we'll run into the same issue with SkShader. Actually, no. The client doesn't need to know anything about SkSmallAllocator for my use case. The SkSmallAllocator is put on the stack in SkDraw's draw calls (currently for just the blitter). At the end of the draw the blitter (and the SkShader::Context) are no longer needed, and are popped off the stack. Likewise, I don't think you need SkSmallAllocator to be public in order to use it. It appears that the only place you create your context object is in AutoDrawLooper in SkCanvas.cpp and in SkDrawLooper.cpp for computing fast bounds. In both cases you can put the SkSmallAllocator in a .cpp file in src/core, where it can see the private class. (It's also okay for tests to use private headers in src/core.)
On 2014/03/10 15:26:39, scroggo wrote: > On 2014/03/10 13:12:17, Dominik Grewe wrote: > > I tried to rebase this patch and use Leon's new SkSmallAllocator instead of a > > raw pointer. That would require for SkSmallAllocator to be publicly visible > > though, as we want users to pass it in to SkDrawLooper::init(). Is there a > > fundamental reason why SkSmallAllocator is not public? Or should I just stick > to > > using a raw storage pointer? > > The reason I didn't make it public was for a few reasons: > - It is designed for a very specific purpose > - Its API may continue to evolve > - It's an optimization rather than providing a feature that allows you to use > graphics in a program > - I didn't see a need for it to be public (which doesn't mean there isn't one or > couldn't be one) > > > > > Btw, I guess we'll run into the same issue with SkShader. > > Actually, no. The client doesn't need to know anything about SkSmallAllocator > for my use case. The > SkSmallAllocator is put on the stack in SkDraw's draw calls (currently for just > the blitter). At the > end of the draw the blitter (and the SkShader::Context) are no longer needed, > and are popped off the > stack. > > Likewise, I don't think you need SkSmallAllocator to be public in order to use > it. It appears that > the only place you create your context object is in AutoDrawLooper in > SkCanvas.cpp and in > SkDrawLooper.cpp for computing fast bounds. In both cases you can put the > SkSmallAllocator in a > .cpp file in src/core, where it can see the private class. (It's also okay for > tests to use private > headers in src/core.) Okay, but then we'd have to make parts of the SkDrawLooper API private, right? Currently we'd have to pass the allocator to SkDrawLooper::init() which is public but never called by Chrome itself. So maybe we can have a public SkDrawLooper and a private SkDrawLooperImpl?
On 2014/03/10 15:49:07, Dominik Grewe wrote: > On 2014/03/10 15:26:39, scroggo wrote: > > On 2014/03/10 13:12:17, Dominik Grewe wrote: > > > I tried to rebase this patch and use Leon's new SkSmallAllocator instead of > a > > > raw pointer. That would require for SkSmallAllocator to be publicly visible > > > though, as we want users to pass it in to SkDrawLooper::init(). Is there a > > > fundamental reason why SkSmallAllocator is not public? Or should I just > stick > > to > > > using a raw storage pointer? > > > > The reason I didn't make it public was for a few reasons: > > - It is designed for a very specific purpose > > - Its API may continue to evolve > > - It's an optimization rather than providing a feature that allows you to use > > graphics in a program > > - I didn't see a need for it to be public (which doesn't mean there isn't one > or > > couldn't be one) > > > > > > > > Btw, I guess we'll run into the same issue with SkShader. > > > > Actually, no. The client doesn't need to know anything about SkSmallAllocator > > for my use case. The > > SkSmallAllocator is put on the stack in SkDraw's draw calls (currently for > just > > the blitter). At the > > end of the draw the blitter (and the SkShader::Context) are no longer needed, > > and are popped off the > > stack. > > > > Likewise, I don't think you need SkSmallAllocator to be public in order to use > > it. It appears that > > the only place you create your context object is in AutoDrawLooper in > > SkCanvas.cpp and in > > SkDrawLooper.cpp for computing fast bounds. In both cases you can put the > > SkSmallAllocator in a > > .cpp file in src/core, where it can see the private class. (It's also okay for > > tests to use private > > headers in src/core.) > > Okay, but then we'd have to make parts of the SkDrawLooper API private, right? > Currently we'd have to pass the allocator to SkDrawLooper::init() which is > public but never called by Chrome itself. So maybe we can have a public > SkDrawLooper and a private SkDrawLooperImpl? My mistake; you are correct, you do need SkSmallAllocator to be public, as does SkShader. I'm okay with making it public then. I'm sure reed@ will look at it with a more API-critical eye.
I've updated the patch to use SkSmallAllocator (now public). I know we said to pause this for a while until we've worked out how to deal with similar classes (mainly SkShader, I guess), but I'd love to get this landed to avoid bit rot. https://codereview.chromium.org/155513012/diff/760001/include/core/SkDrawLoop... File include/core/SkDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/760001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:65: typedef SkSmallAllocator<1, 2 * SkAlign8(sizeof(void*) + sizeof(int))> ContextAllocator; This isn't particularly elegant because of the fixed maximum size for the allocator. But I don't see another way that doesn't require exposing SkSmallAllocator's template parameters to the user (and I think we'd have to expose them for SkDrawLooper itself).
If we added a virtual size_t iNeedThisMuchMemory() to SkDrawLooper, perhaps we wouldn't need to expose anything about *how* we allocated the space. In that world, init(...) could just be given the memory (or memory+size).
On 2014/03/10 18:52:02, Dominik Grewe wrote:
> I've updated the patch to use SkSmallAllocator (now public). I know we said to
> pause this for a while until we've worked out how to deal with similar classes
> (mainly SkShader, I guess), but I'd love to get this landed to avoid bit rot.
I think we just wanted to make sure we were considering the more complicated
cases
so that everything works similarly. Now that we've given some thought about
SkShader,
I think it's fine to flesh out the other cases.
On 2014/03/10 19:10:15, reed1 wrote:
> If we added a virtual
>
> size_t iNeedThisMuchMemory()
>
> to SkDrawLooper, perhaps we wouldn't need to expose anything about *how* we
> allocated the space. In that world, init(...) could just be given the memory
(or
> memory+size).
I like that idea. Then we can make SkSmallAllocator private again. We'll need to
expose its
reserveT method, which I think is fine. We'll also need to modify it a little
bit. The
caller (in this case, the caller of init(), e.g. AutoDrawLooper) knows that it
needs an
SkDrawLooper::Context, but doesn't know how big it will be. For the
SkShader::Context, I
had imagined adding a size field to reserveT, to denote the extra space needed:
template class SkSmallAllocator {
.
.
.
template<class T> void* reserveT(size_t extraSpace);
.
.
.
};
The new version reserves the size of a T plus extraSpace. In my imagined use
case, part of
the extra space is for the buffer used by the subclasses. But in this case, the
extra space
is actually needed for the full subclass of SkDrawLooper::DrawContext (and
SkShader could
also follow this model). So the client will look something like the following:
const size_t extraSpace = looper->iNeedThisMuchMemory() -
sizeof(SkDrawLooper::Context);
void* buffer = fContextAllocator->reserveT<SkDrawLooper::Context>(extraSpace);
fLooperContext = looper->init(..., buffer);
(init() will then call SkNEW_PLACEMENT or SkNEW_PLACEMENT_ARGS as desired.)
This way fContextAllocator can still handle calling the destructor (which is
virtual, so the
correct one will be called). It would be simpler on the client if the API took
totalSpace
instead of extraSpace (I guess we'd just assert that totalSpace >= sizeof(T)).
I also think it's unnecessary to pass the size to init(), so long as we
guarantee that the caller
and implementation of init() will respect iNeedThisMuchMemory().
https://codereview.chromium.org/155513012/diff/760001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):
https://codereview.chromium.org/155513012/diff/760001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:394: SkDrawLooper* fLooper;
After initialization, fLooper is only used to check if it's NULL. If it's NULL,
fLooperContext is also NULL. Can we drop fLooper and check against
fLooperContext?
https://codereview.chromium.org/155513012/diff/760001/tests/QuickRejectTest.cpp
File tests/QuickRejectTest.cpp (right):
https://codereview.chromium.org/155513012/diff/760001/tests/QuickRejectTest.c...
tests/QuickRejectTest.cpp:21: int tp=sizeof(TestDrawLooperContext);
Shouldn't the return value of sizeof be a size_t?
https://codereview.chromium.org/155513012/diff/760001/tests/QuickRejectTest.c...
tests/QuickRejectTest.cpp:22: if (tp < 0) return NULL;
I don't think this should ever happen.
I might be missing something here, but a virtual iNeedThisMuchMemory() is obviously dynamic, so how are we going to statically allocate memory based on that? The whole reason for providing the storage to init() is to avoid heap-allocating the context, right? https://codereview.chromium.org/155513012/diff/760001/tests/QuickRejectTest.cpp File tests/QuickRejectTest.cpp (right): https://codereview.chromium.org/155513012/diff/760001/tests/QuickRejectTest.c... tests/QuickRejectTest.cpp:22: if (tp < 0) return NULL; On 2014/03/10 19:41:32, scroggo wrote: > I don't think this should ever happen. Sorry, please ignore those two lines. I just added them temporarily for debugging. I'll remove them. Thanks for spotting!
On 2014/03/10 21:54:06, Dominik Grewe wrote: > The whole reason for providing the storage to init() is to avoid > heap-allocating the context, right? Yes, the goal is to avoid heap allocation. That said, SkSmallAllocator has an option for using heap allocation as a backup. Another goal of SkSmallAllocator is to make the client's life easier - even if you did have to fall back to heap allocation, the SkSmallAllocator takes care of the free for you. > I might be missing something here, but a virtual iNeedThisMuchMemory() is > obviously dynamic, so how are we going to statically allocate memory based on > that? By knowing what all the callers are. SkSmallAllocator::reserveT is templated by size, but will succeed even if we didn't statically allocate enough space. But it has an assert, so that we can catch it and then adjust kTotalBytes to be large enough to (statically) handle all of the known use cases (i.e. without falling back to the heap).
Okay, here's the latest version. SkSmallAllocator is private again, but exposes reserveT<>(storageSize). SkDrawLooper::init() expects a raw pointer and assumes it has sufficient space for the context (size can be queried by virtual contextSize() method). PTAL. https://codereview.chromium.org/155513012/diff/760001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/155513012/diff/760001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:394: SkDrawLooper* fLooper; On 2014/03/10 19:41:32, scroggo wrote: > After initialization, fLooper is only used to check if it's NULL. If it's NULL, > fLooperContext is also NULL. Can we drop fLooper and check against > fLooperContext? Done.
Okay, here's the latest version. SkSmallAllocator is private again, but exposes reserveT<>(storageSize). SkDrawLooper::init() expects a raw pointer and assumes it has sufficient space for the context (size can be queried by virtual contextSize() method). PTAL.
lgtm https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... File include/core/SkDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:34: * When the context is no longer needed, call cleanup() to ensure memory No need for comment about cleanup() https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:72: * Returns the number of bytes needed to store a Context object. I think it might be clearer to state that this is the number of bytes needed to store the subclass's subclass of Context https://codereview.chromium.org/155513012/diff/820001/src/core/SkSmallAllocat... File src/core/SkSmallAllocator.h (right): https://codereview.chromium.org/155513012/diff/820001/src/core/SkSmallAllocat... src/core/SkSmallAllocator.h:102: * Either way, this class will call ~T() in its destructor and free the heap Maybe it's clear enough, but I wonder if we should be more explicit that if the caller does not create a T in the returned buffer, there will be problems, since we'll call ~T() anyway.
https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... File include/core/SkDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:69: virtual Context* init(SkCanvas*, void* storage) const = 0; Rename to something that reflects its "factory" nature? newContext(...), createContext(...)? Did you consider passing the size as well, just for debugging/security reasons? ... size_t sizeOfStorage) https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:87: virtual bool canComputeFastBounds(const SkPaint& paint); What is the plan for these? Will they become const?
https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... File include/core/SkDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:69: virtual Context* init(SkCanvas*, void* storage) const = 0; On 2014/03/11 14:23:31, reed1 wrote: > Did you consider passing the size as well, just for debugging/security reasons? > > ... size_t sizeOfStorage) Personally I like not having the size there. Passing the size gives the impression that the size might not be big enough, so we have to potentially handle that case separately. (It's also not really a guarantee - someone could call this function with a different size than they actually provided. Granted, it does give us the opportunity to SkASSERT that the size is big enough.)
Picture serialization/hardening started to adopt the pattern of passing in explicit buffer sizes... but I'm not entirely sure of the rational. Sugoi, do you recall?
On 2014/03/11 15:56:30, reed1 wrote: > Picture serialization/hardening started to adopt the pattern of passing in > explicit buffer sizes... but I'm not entirely sure of the rational. Sugoi, do > you recall? If this is the case I'm thinking about, the reason for this was to be able to check how much memory we're about to read from the readbuffer to avoid reading too much memory and performing out-of-bounds memory reads. Some of these issues were originally found by clusterfuzz.
On 2014/03/11 16:53:03, sugoi wrote: > On 2014/03/11 15:56:30, reed1 wrote: > > Picture serialization/hardening started to adopt the pattern of passing in > > explicit buffer sizes... but I'm not entirely sure of the rational. Sugoi, do > > you recall? > > If this is the case I'm thinking about, the reason for this was to be able to > check how much memory we're about to read from the readbuffer to avoid reading > too much memory and performing out-of-bounds memory reads. Some of these issues > were originally found by clusterfuzz. That makes sense. That shouldn't be an issue here, where the size of the buffer is determined by querying the subclass, rather than from reading a stream.
Thanks for the feedback. https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... File include/core/SkDrawLooper.h (right): https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:34: * When the context is no longer needed, call cleanup() to ensure memory On 2014/03/11 14:08:05, scroggo wrote: > No need for comment about cleanup() Done. https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:69: virtual Context* init(SkCanvas*, void* storage) const = 0; On 2014/03/11 14:23:31, reed1 wrote: > Rename to something that reflects its "factory" nature? newContext(...), > createContext(...)? Done. > Did you consider passing the size as well, just for debugging/security reasons? > > ... size_t sizeOfStorage) As scroggo@ suggested, I'm not passing the size here. https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:72: * Returns the number of bytes needed to store a Context object. On 2014/03/11 14:08:05, scroggo wrote: > I think it might be clearer to state that this is the number of bytes needed to > store the subclass's subclass of Context Done. https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... include/core/SkDrawLooper.h:87: virtual bool canComputeFastBounds(const SkPaint& paint); On 2014/03/11 14:23:31, reed1 wrote: > What is the plan for these? Will they become const? Good point. Yes, they can now be const.
On 2014/03/11 18:28:16, Dominik Grewe wrote: > Thanks for the feedback. > > https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... > File include/core/SkDrawLooper.h (right): > > https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... > include/core/SkDrawLooper.h:34: * When the context is no longer needed, call > cleanup() to ensure memory > On 2014/03/11 14:08:05, scroggo wrote: > > No need for comment about cleanup() > > Done. > > https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... > include/core/SkDrawLooper.h:69: virtual Context* init(SkCanvas*, void* storage) > const = 0; > On 2014/03/11 14:23:31, reed1 wrote: > > Rename to something that reflects its "factory" nature? newContext(...), > > createContext(...)? > > Done. > > > Did you consider passing the size as well, just for debugging/security > reasons? > > > > ... size_t sizeOfStorage) > > As scroggo@ suggested, I'm not passing the size here. > > https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... > include/core/SkDrawLooper.h:72: * Returns the number of bytes needed to store a > Context object. > On 2014/03/11 14:08:05, scroggo wrote: > > I think it might be clearer to state that this is the number of bytes needed > to > > store the subclass's subclass of Context > > Done. > > https://codereview.chromium.org/155513012/diff/820001/include/core/SkDrawLoop... > include/core/SkDrawLooper.h:87: virtual bool canComputeFastBounds(const SkPaint& > paint); > On 2014/03/11 14:23:31, reed1 wrote: > > What is the plan for these? Will they become const? > > Good point. Yes, they can now be const. lgtm
lgtm
The CQ bit was checked by dominikg@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/dominikg@chromium.org/155513012/840001
Message was sent while issue was closed.
Change committed as 13760 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
