Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
742253002:
remove GrAllocPool (Closed)
|
Created:
6 years, 1 month ago by joshua.litt Modified:
6 years, 1 month ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Project:
skia Visibility:
Public. |
Descriptionremove GrAllocPool
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/947556f6583e62b1ae19dcda94e0dea78babda2b
Patch Set 1 #Patch Set 2 : update gyp #Patch Set 3 : update to use SkVarAlloc #
Total comments: 3
Patch Set 4 : feedback incorporated #
Total comments: 1
Patch Set 5 : feedback inc #
Messages
Total messages: 29 (6 generated)
joshualitt@google.com changed reviewers: + bsalomon@google.com, joshualitt@google.com
Turns out it was used in GrTextStrike
bsalomon@google.com changed reviewers: + jvanverth@google.com
I'm not familiar with this use and not sure if grmemorypool is a good substitute.
On 2014/11/20 22:21:55, bsalomon wrote: > I'm not familiar with this use and not sure if grmemorypool is a good > substitute. TextStrike's live in the fontcache and cache individual glyphs using a glyphcache which holds pointers to a memory pool. When a given glyph is requested from a textstrike, if it exists in the cache then great, otherwise the textstrike will create the glyph from its memory pool and put it into its glyph cache. The pool allocation only grows, it is never pruned, presumably because fonts are pretty finite. When a textstrike itself is evicted from the fontcache, it purges its cache and memory allocation pool completely. Is there something better than memory pool for this purpose? (Adding Jim to correct me if I got any of the font stuff wrong)
On 2014/11/20 22:39:34, joshualitt wrote: > On 2014/11/20 22:21:55, bsalomon wrote: > > I'm not familiar with this use and not sure if grmemorypool is a good > > substitute. > > TextStrike's live in the fontcache and cache individual glyphs using a > glyphcache which holds pointers to a memory pool. When a given glyph is > requested from a textstrike, if it exists in the cache then great, otherwise the > textstrike will create the glyph from its memory pool and put it into its glyph > cache. The pool allocation only grows, it is never pruned, presumably because > fonts are pretty finite. When a textstrike itself is evicted from the > fontcache, it purges its cache and memory allocation pool completely. > > Is there something better than memory pool for this purpose? > > (Adding Jim to correct me if I got any of the font stuff wrong) Actually now that I wrote all of that out, is there any reason the textstrike doesn't just use a stack of glyphs?
On 2014/11/20 22:46:59, joshualitt wrote: > On 2014/11/20 22:39:34, joshualitt wrote: > > On 2014/11/20 22:21:55, bsalomon wrote: > > > I'm not familiar with this use and not sure if grmemorypool is a good > > > substitute. > > > > TextStrike's live in the fontcache and cache individual glyphs using a > > glyphcache which holds pointers to a memory pool. When a given glyph is > > requested from a textstrike, if it exists in the cache then great, otherwise > the > > textstrike will create the glyph from its memory pool and put it into its > glyph > > cache. The pool allocation only grows, it is never pruned, presumably because > > fonts are pretty finite. When a textstrike itself is evicted from the > > fontcache, it purges its cache and memory allocation pool completely. > > > > Is there something better than memory pool for this purpose? > > > > (Adding Jim to correct me if I got any of the font stuff wrong) > > Actually now that I wrote all of that out, is there any reason the textstrike > doesn't just use a stack of glyphs? err, that would obviously only work if we knew how many glyphs were in a font when creating a textstrike. If we don't know that number before hand we can't use a growable stack.
On 2014/11/20 22:50:54, joshualitt wrote: > On 2014/11/20 22:46:59, joshualitt wrote: > > On 2014/11/20 22:39:34, joshualitt wrote: > > > On 2014/11/20 22:21:55, bsalomon wrote: > > > > I'm not familiar with this use and not sure if grmemorypool is a good > > > > substitute. > > > > > > TextStrike's live in the fontcache and cache individual glyphs using a > > > glyphcache which holds pointers to a memory pool. When a given glyph is > > > requested from a textstrike, if it exists in the cache then great, otherwise > > the > > > textstrike will create the glyph from its memory pool and put it into its > > glyph > > > cache. The pool allocation only grows, it is never pruned, presumably > because > > > fonts are pretty finite. When a textstrike itself is evicted from the > > > fontcache, it purges its cache and memory allocation pool completely. > > > > > > Is there something better than memory pool for this purpose? > > > > > > (Adding Jim to correct me if I got any of the font stuff wrong) > > > > Actually now that I wrote all of that out, is there any reason the textstrike > > doesn't just use a stack of glyphs? > > err, that would obviously only work if we knew how many glyphs were in a font > when creating a textstrike. If we don't know that number before hand we can't > use a growable stack. ...and then I remembered there are fonts with many many characters, and we obviously wouldn't want to preallocate all of a chinese font to print a few characters. Some kind of pool is obviously required. Nothing to see here, carry on.
On 2014/11/20 22:54:20, joshualitt wrote: > On 2014/11/20 22:50:54, joshualitt wrote: > > On 2014/11/20 22:46:59, joshualitt wrote: > > > On 2014/11/20 22:39:34, joshualitt wrote: > > > > On 2014/11/20 22:21:55, bsalomon wrote: > > > > > I'm not familiar with this use and not sure if grmemorypool is a good > > > > > substitute. > > > > > > > > TextStrike's live in the fontcache and cache individual glyphs using a > > > > glyphcache which holds pointers to a memory pool. When a given glyph is > > > > requested from a textstrike, if it exists in the cache then great, > otherwise > > > the > > > > textstrike will create the glyph from its memory pool and put it into its > > > glyph > > > > cache. The pool allocation only grows, it is never pruned, presumably > > because > > > > fonts are pretty finite. When a textstrike itself is evicted from the > > > > fontcache, it purges its cache and memory allocation pool completely. > > > > > > > > Is there something better than memory pool for this purpose? > > > > > > > > (Adding Jim to correct me if I got any of the font stuff wrong) > > > > > > Actually now that I wrote all of that out, is there any reason the > textstrike > > > doesn't just use a stack of glyphs? > > > > err, that would obviously only work if we knew how many glyphs were in a font > > when creating a textstrike. If we don't know that number before hand we can't > > use a growable stack. > > ...and then I remembered there are fonts with many many characters, and we > obviously wouldn't want to preallocate all of a chinese font to print a few > characters. Some kind of pool is obviously required. Nothing to see here, > carry on. Maybe one of SkChunkAllocator or the new thing (SkVarAlloc?) are a better match. GrMemoryPool is built to be able to support random deletes and freeing of abandoned chunks.
joshualitt@google.com changed reviewers: + mtklein@google.com
Adding Mike who wrote VarAlloc. GrAllocPool was also a variable size allocator. It seems like we might benefit from a const size allocator, but I think that is only worth doing if this shows up in profiling.
On 2014/11/21 14:46:53, joshualitt wrote: > Adding Mike who wrote VarAlloc. > > GrAllocPool was also a variable size allocator. It seems like we might benefit > from a const size allocator, but I think that is only worth doing if this shows > up in profiling. So far allocating GrGlyphs in GrTextStrike hasn't shown up as a significant performance hit when rendering text.
What's the goal here? Faster, better ram usage, less code?
https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.cpp File src/gpu/GrTextStrike.cpp (right): https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.cpp... src/gpu/GrTextStrike.cpp:270: GrGlyph* glyph = (GrGlyph*)fPool.alloc(sizeof(GrGlyph), 0); Are you sure you don't want SK_MALLOC_THROW here? You're not checking glyph for null.
https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.h File src/gpu/GrTextStrike.h (right): https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.h#n... src/gpu/GrTextStrike.h:65: SkVarAlloc fPool; SkGlyphs look relatively small and relocatable. If you're keen to play around, I'd consider just putting them in an SkTDArray, referring to them by int index instead of pointer if need be. It's hard to beat the density of SkTDArray, and for things this small I wouldn't be surprised if realloc were faster than doing block allocation like GrTAllocPool/SkVarAlloc/SkChunkAlloc.
https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.h File src/gpu/GrTextStrike.h (right): https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.h#n... src/gpu/GrTextStrike.h:65: SkVarAlloc fPool; On 2014/11/21 15:20:22, mtklein wrote: > SkGlyphs look relatively small and relocatable. If you're keen to play around, > I'd consider just putting them in an SkTDArray, referring to them by int index > instead of pointer if need be. It's hard to beat the density of SkTDArray, and > for things this small I wouldn't be surprised if realloc were faster than doing > block allocation like GrTAllocPool/SkVarAlloc/SkChunkAlloc. Err, was looking at SkGlyph. Looking at GrGlyph, all the same applies. Small, relocatable, etc.
On 2014/11/21 14:55:38, mtklein wrote: > What's the goal here? Faster, better ram usage, less code? Less code. I believe GrAllocPool only exists b/c originally Ganesh couldn't use Sk-code. I think it was originally a workalike to SkC.A.
On 2014/11/21 15:21:59, mtklein wrote: > https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.h > File src/gpu/GrTextStrike.h (right): > > https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.h#n... > src/gpu/GrTextStrike.h:65: SkVarAlloc fPool; > On 2014/11/21 15:20:22, mtklein wrote: > > SkGlyphs look relatively small and relocatable. If you're keen to play > around, > > I'd consider just putting them in an SkTDArray, referring to them by int index > > instead of pointer if need be. It's hard to beat the density of SkTDArray, > and > > for things this small I wouldn't be surprised if realloc were faster than > doing > > block allocation like GrTAllocPool/SkVarAlloc/SkChunkAlloc. > > Err, was looking at SkGlyph. Looking at GrGlyph, all the same applies. Small, > relocatable, etc. So what is the difference between SkVarAlloc and SkChunkAlloc? Do we need both?
On 2014/11/21 15:34:06, jvanverth1 wrote: > On 2014/11/21 15:21:59, mtklein wrote: > > https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.h > > File src/gpu/GrTextStrike.h (right): > > > > > https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.h#n... > > src/gpu/GrTextStrike.h:65: SkVarAlloc fPool; > > On 2014/11/21 15:20:22, mtklein wrote: > > > SkGlyphs look relatively small and relocatable. If you're keen to play > > around, > > > I'd consider just putting them in an SkTDArray, referring to them by int > index > > > instead of pointer if need be. It's hard to beat the density of SkTDArray, > > and > > > for things this small I wouldn't be surprised if realloc were faster than > > doing > > > block allocation like GrTAllocPool/SkVarAlloc/SkChunkAlloc. > > > > Err, was looking at SkGlyph. Looking at GrGlyph, all the same applies. > Small, > > relocatable, etc. > > So what is the difference between SkVarAlloc and SkChunkAlloc? Do we need both? Hopefully not. VarAlloc is new, more efficient where its been used so far (just picture). VarAlloc will actually be a closer fit to the allocation pattern in the deleted code. Chunkalloc will probably start way Yoo big for this. If it's just about sharing code, I'd try varallocn with SK_MALLOC_THROW.
On 2014/11/21 15:39:05, mtklein wrote: > On 2014/11/21 15:34:06, jvanverth1 wrote: > > On 2014/11/21 15:21:59, mtklein wrote: > > > https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.h > > > File src/gpu/GrTextStrike.h (right): > > > > > > > > > https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.h#n... > > > src/gpu/GrTextStrike.h:65: SkVarAlloc fPool; > > > On 2014/11/21 15:20:22, mtklein wrote: > > > > SkGlyphs look relatively small and relocatable. If you're keen to play > > > around, > > > > I'd consider just putting them in an SkTDArray, referring to them by int > > index > > > > instead of pointer if need be. It's hard to beat the density of > SkTDArray, > > > and > > > > for things this small I wouldn't be surprised if realloc were faster than > > > doing > > > > block allocation like GrTAllocPool/SkVarAlloc/SkChunkAlloc. > > > > > > Err, was looking at SkGlyph. Looking at GrGlyph, all the same applies. > > Small, > > > relocatable, etc. > > > > So what is the difference between SkVarAlloc and SkChunkAlloc? Do we need > both? > > Hopefully not. VarAlloc is new, more efficient where its been used so far (just > picture). > > VarAlloc will actually be a closer fit to the allocation pattern in the deleted > code. Chunkalloc will probably start way Yoo big for this. > > If it's just about sharing code, I'd try varallocn with SK_MALLOC_THROW. (Snet form my nandroid.)
On 2014/11/21 15:41:15, mtklein wrote: > On 2014/11/21 15:39:05, mtklein wrote: > > On 2014/11/21 15:34:06, jvanverth1 wrote: > > > On 2014/11/21 15:21:59, mtklein wrote: > > > > > https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.h > > > > File src/gpu/GrTextStrike.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/742253002/diff/40001/src/gpu/GrTextStrike.h#n... > > > > src/gpu/GrTextStrike.h:65: SkVarAlloc fPool; > > > > On 2014/11/21 15:20:22, mtklein wrote: > > > > > SkGlyphs look relatively small and relocatable. If you're keen to play > > > > around, > > > > > I'd consider just putting them in an SkTDArray, referring to them by int > > > index > > > > > instead of pointer if need be. It's hard to beat the density of > > SkTDArray, > > > > and > > > > > for things this small I wouldn't be surprised if realloc were faster > than > > > > doing > > > > > block allocation like GrTAllocPool/SkVarAlloc/SkChunkAlloc. > > > > > > > > Err, was looking at SkGlyph. Looking at GrGlyph, all the same applies. > > > Small, > > > > relocatable, etc. > > > > > > So what is the difference between SkVarAlloc and SkChunkAlloc? Do we need > > both? > > > > Hopefully not. VarAlloc is new, more efficient where its been used so far > (just > > picture). > > > > VarAlloc will actually be a closer fit to the allocation pattern in the > deleted > > code. Chunkalloc will probably start way Yoo big for this. > > > > If it's just about sharing code, I'd try varallocn with SK_MALLOC_THROW. > > (Snet form my nandroid.) feedback incorporated.
https://codereview.chromium.org/742253002/diff/60001/src/gpu/GrMemoryPool.h File src/gpu/GrMemoryPool.h (right): https://codereview.chromium.org/742253002/diff/60001/src/gpu/GrMemoryPool.h#n... src/gpu/GrMemoryPool.h:41: void releaseAll(); You may want to remove this, as you no longer need it.
On 2014/11/21 16:11:41, jvanverth1 wrote: > https://codereview.chromium.org/742253002/diff/60001/src/gpu/GrMemoryPool.h > File src/gpu/GrMemoryPool.h (right): > > https://codereview.chromium.org/742253002/diff/60001/src/gpu/GrMemoryPool.h#n... > src/gpu/GrMemoryPool.h:41: void releaseAll(); > You may want to remove this, as you no longer need it. good catch, updated
lgtm
The CQ bit was checked by joshualitt@google.com
The CQ bit was unchecked by joshualitt@google.com
The CQ bit was checked by joshualitt@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/742253002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/947556f6583e62b1ae19dcda94e0dea78babda2b |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
