Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
761313004:
Add a small LRU cache for the JBIG2 symbol dictionary. (Closed)
DescriptionAdd a small LRU cache for the JBIG2 symbol dictionary.
This reduces rendering time on my test document by over
10 seconds. It is super common for a JBIG2 dictionary to
span multiple pages, so we don't want to decode the same
dictionary over and over again.
Original patch from Jeff Breidenbach (breidenbach@gmail.com)
BUG=https://code.google.com/p/pdfium/issues/detail?id=85
Patch Set 1 : intial implementation #
Total comments: 16
Patch Set 2 : use list instead of vector #
Total comments: 2
Patch Set 3 : fix memory leak by calling destructor #Patch Set 4 : indent with 4 spaces #
Total comments: 6
Patch Set 5 : use list of pairs #
Total comments: 4
Patch Set 6 : use typedef #
Total comments: 16
Patch Set 7 : style changes, tidy up comments #Patch Set 8 : style changes, fix flaw with iterator #
Messages
Total messages: 45 (4 generated)
thestig@chromium.org changed reviewers: + bo_xu@foxitsoftware.com, jun_fang@foxitsoftware.com, thestig@chromium.org
https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_Context.cpp:815: cache_keys.erase(cache_keys.begin()); Should line 815 be: "cache_keys.erase(it);" ? I guess this will affect your test timing? Nit: The for loop could start from cache_keys.rbegin() as this is the most recently used one. https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_Context.cpp:839: CJBig2_SymbolDict *value = pSegment->m_Result.sd->DeepCopy(); should |value| be deleted after use? https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... File core/src/fxcodec/jbig2/JBig2_SymbolDict.cpp (right): https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_SymbolDict.cpp:18: CJBig2_SymbolDict *dst; CJBig2_SymbolDict *dst = NULL https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_SymbolDict.cpp:20: if (src->m_bContextRetained || Nit: maybe can set the tab space = 4 to match the pdfium style for now, although there is a plan to convert pdfium to chromium style later. https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_SymbolDict.cpp:31: CJBig2_Image(*(src->SDEXSYMS[i]))); Seems only memory alloc but not actually copied, do I miss something?
In the tile and description of the issue, can you add a new line after "Add a small LRU cache for the JBIG2 symbol dictionary.". Otherwise everything will be in the commit title. https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_Context.cpp:816: break; Or can we use std:list to replace the std:vector here, so getting the key takes const time.
Need help on freeing memory (see one of the comments). I also want to know if the cache is in the right place. I imagine some people disliking static variables because they can mess with thread safety. https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_Context.cpp:815: cache_keys.erase(cache_keys.begin()); Done. (I made the front hold the freshest entries) https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_Context.cpp:816: break; On 2014/12/07 19:22:50, Bo Xu wrote: > Or can we use std:list to replace the std:vector here, so getting the key takes > const time. Done. https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_Context.cpp:839: CJBig2_SymbolDict *value = pSegment->m_Result.sd->DeepCopy(); That you. I think I need to free the memory when we remove the entry from the cache. I am not sure how to do this. Help! https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... File core/src/fxcodec/jbig2/JBig2_SymbolDict.cpp (right): https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_SymbolDict.cpp:18: CJBig2_SymbolDict *dst; On 2014/12/06 05:59:43, Bo Xu wrote: > CJBig2_SymbolDict *dst = NULL Done. https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_SymbolDict.cpp:20: if (src->m_bContextRetained || On 2014/12/06 05:59:43, Bo Xu wrote: > Nit: maybe can set the tab space = 4 to match the pdfium style for now, although > there is a plan to convert pdfium to chromium style later. Done. https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_SymbolDict.cpp:31: CJBig2_Image(*(src->SDEXSYMS[i]))); On 2014/12/06 05:59:43, Bo Xu wrote: > Seems only memory alloc but not actually copied, do I miss something? I am following the example in line 2566 of JBig2_GeneralDecoder.cpp. CJBig2_Image() is a copy constructor. And this seems to work. But please double check this; I am very new to this codebase and don't really know what I am doing. https://github.com/coolwanglu/PDFium.js/blob/master/core/src/fxcodec/jbig2/JB...
Just wonder what is the performance increase by changing the cache size, like using 3, 4...? https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_Context.cpp:839: CJBig2_SymbolDict *value = pSegment->m_Result.sd->DeepCopy(); On 2014/12/08 21:00:02, breidenbach wrote: > That you. I think I need to free the memory when we remove the entry from the > cache. I am not sure how to do this. Help! How about making the cache member variable of the CJBig2_Context class and let the destructor do the job? https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... File core/src/fxcodec/jbig2/JBig2_SymbolDict.cpp (right): https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_SymbolDict.cpp:31: CJBig2_Image(*(src->SDEXSYMS[i]))); That's right, I missed that. https://codereview.chromium.org/761313004/diff/20001/core/src/fxcodec/jbig2/J... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/20001/core/src/fxcodec/jbig2/J... core/src/fxcodec/jbig2/JBig2_Context.cpp:811: for(std::list<FX_BYTE*>::iterator it = cache_keys.begin(); Nit: set tab size to 4
On 2014/12/08 23:00:17, Bo Xu wrote: > Just wonder what is the performance increase by changing the cache size, like > using 3, 4...? For experiment.pdf, the dictionaries are like this: A B A C A D A E A F ... Therefore there is no benefit of increasing the cache size beyond 2 if the pages are rendered in order. I expect that for other JBIG2 content, the dictionaries are more likely to look like this. A A A A A A A A A A B B B B B B B B B B C C C C C C C C C C ... Which will have perfect performance with cache size 1. I do not expect any need for a cache larger than 2, unless we are rendering pages out of order. Finally, I want to make sure you saw my question about using a static variable to hold the cache. Is that okay?
https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_Context.cpp:839: CJBig2_SymbolDict *value = pSegment->m_Result.sd->DeepCopy(); I made an explicit call to delete. Is that okay? I think it is invoking the destructor and my memory checking tool does not seem to get upset. https://codereview.chromium.org/761313004/diff/20001/core/src/fxcodec/jbig2/J... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/20001/core/src/fxcodec/jbig2/J... core/src/fxcodec/jbig2/JBig2_Context.cpp:811: for(std::list<FX_BYTE*>::iterator it = cache_keys.begin(); On 2014/12/08 23:00:17, Bo Xu wrote: > Nit: set tab size to 4 Done.
https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/1/core/src/fxcodec/jbig2/JBig2... core/src/fxcodec/jbig2/JBig2_Context.cpp:839: CJBig2_SymbolDict *value = pSegment->m_Result.sd->DeepCopy(); On 2014/12/08 23:21:04, breidenbach wrote: > I made an explicit call to delete. Is that okay? I think > it is invoking the destructor and my memory checking tool > does not seem to get upset. > Sure, the explicit delete is ok here. I am thinking moving the static variables into CJBig2_Context, would that be sufficient to make them available for the whole document?
> I am thinking moving the static variables into CJBig2_Context, would that be > sufficient to make them available for the whole document? I tried this earlier, and it didn't work for me. The cache missed every time. Maybe I made a mistake, but I don't think so. I debugged by putting fprintf(stderr, "Cache hit\n") in the appropriate spot.
On 2014/12/08 23:50:25, jab wrote: > > I am thinking moving the static variables into CJBig2_Context, would that be > > sufficient to make them available for the whole document? > > I tried this earlier, and it didn't work for me. The cache > missed every time. Maybe I made a mistake, but I don't think so. > I debugged by putting fprintf(stderr, "Cache hit\n") in the > appropriate spot. You are right. In fx_codec_jbig.cpp StartDecode(), the m_pContext is created there each time the decoding start. The CCodec_Jbig2Module seems to be where the cache should be added to, but that would result in some non-trivial change. We can use the static variable for now. Thanks! LGTM
On 2014/12/09 00:23:05, Bo Xu wrote: > On 2014/12/08 23:50:25, jab wrote: > > > I am thinking moving the static variables into CJBig2_Context, would that be > > > sufficient to make them available for the whole document? > > > > I tried this earlier, and it didn't work for me. The cache > > missed every time. Maybe I made a mistake, but I don't think so. > > I debugged by putting fprintf(stderr, "Cache hit\n") in the > > appropriate spot. > > You are right. In fx_codec_jbig.cpp StartDecode(), the m_pContext is created > there each time the decoding start. > The CCodec_Jbig2Module seems to be where the cache should be added to, but that > would result in some non-trivial change. > We can use the static variable for now. Thanks! > LGTM One more thing. Can you change the "Description" part too? Add new line after the first sentence.
> One more thing. Can you change the "Description" part too? Add new line after > the first sentence. I would love to but cannot figure out how to do it.
On 2014/12/09 01:02:03, jab wrote: > > One more thing. Can you change the "Description" part too? Add new line after > > the first sentence. > > I would love to but cannot figure out how to do it. You can click on left top panel "Edit Issue". The first line in "Description" will be the commit title shown in the repos. The rest will be in the commit message but they won't auto new line. We always have to manually wrap the contents.
The CQ bit was checked by breidenbach@gmail.com
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because it did not recognize the base URL. Please commit your change manually.
On 2014/12/09 18:36:00, I haz the power (commit-bot) wrote: > Commit queue rejected this change because it did not recognize the base URL. > Please commit your change manually. @jab, you have to use "git cl land" to land the patch as the commit-bot does not run for pdfium.
$ git cl land Invalid user name or password. Please generate a new password at: https://pdfium.googlesource.com/new-password [I followed the instructions there] $ git cl land fatal: remote error: Git access forbidden Failed to push. If this persists, please file a bug.
bo_xu@foxitsoftware.com changed reviewers: + tsepez@chromium.org
Oh, seems you still need permission. @Tom, can you advise?
I'm totally cool with someone else submitting this change, if that makes life easier.
On 2014/12/09 20:15:18, jab wrote: > I'm totally cool with someone else submitting this change, if that makes life > easier. I can do that. In the end of the description can you add a line like: "Original patch from name (email)". Replace the name and email with yours :)
done
Jeff, have you signed a chromium contributors agreement?
https://codereview.chromium.org/761313004/diff/60001/core/src/fxcodec/jbig2/J... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/60001/core/src/fxcodec/jbig2/J... core/src/fxcodec/jbig2/JBig2_Context.cpp:18: const int kMaxCacheSize = 2; nit: Let's rename this something like kMaxKBIG2DictionaryCacheSize, since the name "cache" isn't very specific. https://codereview.chromium.org/761313004/diff/60001/core/src/fxcodec/jbig2/J... core/src/fxcodec/jbig2/JBig2_Context.cpp:19: static std::map<FX_BYTE*, CJBig2_SymbolDict*> cache; Others have mentioned it, but let's not add any static memory. We really need to find an object to own this. https://codereview.chromium.org/761313004/diff/60001/core/src/fxcodec/jbig2/J... core/src/fxcodec/jbig2/JBig2_Context.cpp:20: static list<FX_BYTE*> cache_keys; why do we need both a map and a list if there's only going to be a few of these? Having a std::list<std::pair<FX_BYTE*, CJBig2_SymbolDict*>> should be sufficient, since you've indicated that we don't have to crank the size up to get the benefit. This would avoid trying to keep the two data structures in sync with each other.
On 2014/12/09 22:50:55, Tom Sepez wrote: > Jeff, have you signed a chromium contributors agreement? Nevermind. Googlers are covered.
All other changes will be made tomorrow as per reviewer request. https://codereview.chromium.org/761313004/diff/60001/core/src/fxcodec/jbig2/J... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/60001/core/src/fxcodec/jbig2/J... core/src/fxcodec/jbig2/JBig2_Context.cpp:19: static std::map<FX_BYTE*, CJBig2_SymbolDict*> cache; Based on Bo's comment, this requires refactoring beyond my capability as a PDFium beginner. Can I either have a whole bunch of hand holding, or would someone experienced be willing to complete the work? I'm worried that if the answer is no to both questions, then the problem will never be fixed. That's bad because this is a user visible, major performance problem for a certain class of documents.
On 2014/12/10 07:16:17, jab wrote: > All other changes will be made tomorrow as per reviewer request. > > https://codereview.chromium.org/761313004/diff/60001/core/src/fxcodec/jbig2/J... > File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): > > https://codereview.chromium.org/761313004/diff/60001/core/src/fxcodec/jbig2/J... > core/src/fxcodec/jbig2/JBig2_Context.cpp:19: static std::map<FX_BYTE*, > CJBig2_SymbolDict*> cache; > Based on Bo's comment, this requires refactoring beyond my > capability as a PDFium beginner. Can I either have a whole > bunch of hand holding, or would someone experienced be > willing to complete the work? > > I'm worried that if the answer is no to both questions, then > the problem will never be fixed. That's bad because this is a > user visible, major performance problem for a certain class of > documents. Ok. Use the static variable for now, but file a follow-up bug at https://code.google.com/p/pdfium/issues/list so that we can track the additional work required. (and thanks for your efforts, in case that wasn't obvious).
https://codereview.chromium.org/761313004/diff/60001/core/src/fxcodec/jbig2/J... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/60001/core/src/fxcodec/jbig2/J... core/src/fxcodec/jbig2/JBig2_Context.cpp:18: const int kMaxCacheSize = 2; On 2014/12/10 00:14:46, Tom Sepez wrote: > nit: Let's rename this something like kMaxKBIG2DictionaryCacheSize, since the > name "cache" isn't very specific. Done. https://codereview.chromium.org/761313004/diff/60001/core/src/fxcodec/jbig2/J... core/src/fxcodec/jbig2/JBig2_Context.cpp:20: static list<FX_BYTE*> cache_keys; On 2014/12/10 00:14:46, Tom Sepez wrote: > why do we need both a map and a list if there's only going to be a few of these? > Having a std::list<std::pair<FX_BYTE*, CJBig2_SymbolDict*>> should be > sufficient, since you've indicated that we don't have to crank the size up to > get the benefit. This would avoid trying to keep the two data structures in > sync with each other. Done.
https://codereview.chromium.org/761313004/diff/80001/core/src/fxcodec/jbig2/J... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/80001/core/src/fxcodec/jbig2/J... core/src/fxcodec/jbig2/JBig2_Context.cpp:19: static list<pair<FX_BYTE*, CJBig2_SymbolDict*> > symbol_dict_cache; You need std:: https://codereview.chromium.org/761313004/diff/80001/core/src/fxcodec/jbig2/J... core/src/fxcodec/jbig2/JBig2_Context.cpp:19: static list<pair<FX_BYTE*, CJBig2_SymbolDict*> > symbol_dict_cache; I'd add a typedef for "pair<FX_BYTE*, CJBig2_SymbolDict*>" so it's less tedious to type everywhere.
https://codereview.chromium.org/761313004/diff/80001/core/src/fxcodec/jbig2/J... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/80001/core/src/fxcodec/jbig2/J... core/src/fxcodec/jbig2/JBig2_Context.cpp:19: static list<pair<FX_BYTE*, CJBig2_SymbolDict*> > symbol_dict_cache; On 2014/12/12 00:01:20, Lei Zhang wrote: > You need std:: Done. https://codereview.chromium.org/761313004/diff/80001/core/src/fxcodec/jbig2/J... core/src/fxcodec/jbig2/JBig2_Context.cpp:19: static list<pair<FX_BYTE*, CJBig2_SymbolDict*> > symbol_dict_cache; On 2014/12/12 00:01:20, Lei Zhang wrote: > I'd add a typedef for "pair<FX_BYTE*, CJBig2_SymbolDict*>" so it's less tedious > to type everywhere. Done.
Tracking bug filed against use of static memory for cache. https://code.google.com/p/pdfium/issues/detail?id=93
If reviewers are happy, could someone either 1) "land" this changelist for me OR 2) give me permission to "land" this changelist?
On 2014/12/15 17:22:24, jab wrote: > If reviewers are happy, could someone either > > 1) "land" this changelist for me OR > 2) give me permission to "land" this changelist? I can land if others are happy with the changes
https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:15: // list keeps track of the freshness of enties, with freshest ones are typo: enties -> entities grammar: "with freshest ones are at the front" https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:814: symbol_dict_cache.erase(it); Is |it| still valid after you call erase() on it?
Only nits, https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:12: // common for a JBIG2 dictionary to span multiple page in a PDF file, nit: pages https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:13: // and we do not want to decode the same dictionary over an over nit: over and over https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:19: typedef std::pair<FX_BYTE*, CJBig2_SymbolDict*> cache_pair; nit: to match up with the style conventions of this file, this would be called CJBig2_CachePair. https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:20: static std::list<cache_pair> symbol_dict_cache; nit: they're less consistent about naming of globals, though SymbolDictCache would be more consisitent. Your call on either of these. https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:810: for(std::list<cache_pair>::iterator it = nit: auto& it = .. would be more concise here. Still, I'm not a fan of explicit iteration any more when you can do things like auto& it = std::find_if(symbol_dict_cache.begin(), symbol_dict_cache.end(), [key](const cache_pair& element) { return element.first == key; });
https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:12: // common for a JBIG2 dictionary to span multiple page in a PDF file, On 2014/12/15 19:58:14, Tom Sepez wrote: > nit: pages Done. https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:13: // and we do not want to decode the same dictionary over an over On 2014/12/15 19:58:14, Tom Sepez wrote: > nit: over and over Done. https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:15: // list keeps track of the freshness of enties, with freshest ones are On 2014/12/15 19:28:09, Lei Zhang wrote: > typo: enties -> entities > grammar: "with freshest ones are at the front" Done. https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:19: typedef std::pair<FX_BYTE*, CJBig2_SymbolDict*> cache_pair; On 2014/12/15 19:58:14, Tom Sepez wrote: > nit: to match up with the style conventions of this file, this would be called > CJBig2_CachePair. Done. https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:20: static std::list<cache_pair> symbol_dict_cache; On 2014/12/15 19:58:14, Tom Sepez wrote: > nit: they're less consistent about naming of globals, though SymbolDictCache > would be more consisitent. Your call on either of these. Done. https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:810: for(std::list<cache_pair>::iterator it = I'm really sorry, but this code is too sophisticated for me. I don't feel comfortable submitting something that I don't understand. I'm also not confident in my ability to retain understanding over time even if it is clearly explained to me. I respectfully request that we reserve this for a future changelist, to be written by a more sophisticated programmer than myself. https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:814: symbol_dict_cache.erase(it); On 2014/12/15 19:28:09, Lei Zhang wrote: > Is |it| still valid after you call erase() on it? I am not a C++ guru, but I can say that the code compiles and runs correctly on my machine as is. However, I've swapped the two lines anyway. symbol_dict_cache.push_front(*it); symbol_dict_cache.erase(it);
https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:810: for(std::list<cache_pair>::iterator it = please let me know if there is a required change or not. if it is, i will need some assistance.
https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... File core/src/fxcodec/jbig2/JBig2_Context.cpp (right): https://codereview.chromium.org/761313004/diff/100001/core/src/fxcodec/jbig2/... core/src/fxcodec/jbig2/JBig2_Context.cpp:814: symbol_dict_cache.erase(it); On 2014/12/15 21:00:08, jab wrote: > On 2014/12/15 19:28:09, Lei Zhang wrote: > > Is |it| still valid after you call erase() on it? > > I am not a C++ guru, but I can say that the code compiles and runs correctly on > my machine as is. However, I've swapped the two lines anyway. > > symbol_dict_cache.push_front(*it); > symbol_dict_cache.erase(it); I guess I should have said "|it| is not valid after calling erase()" If you look up std::list::erase() it will say the iterator is invalidated. As an example: #include <stdio.h> #include <list> #include <string> int main() { std::list<std::string> mylist; mylist.push_back("a"); mylist.push_back("b"); mylist.push_back("c"); mylist.push_back("d"); for (std::list<std::string>::iterator it = mylist.begin(); it != mylist.end(); ++it) { if (*it == "c") { mylist.erase(it); mylist.push_front(*it); break; } } for (std::list<std::string>::iterator it = mylist.begin(); it != mylist.end(); ++it) { printf("%s\n", it->c_str()); } return 0; } Does not print out: c a b d And if you build with: g++ -D_GLIBCXX_DEBUG=1, it will crash. Swapping works here only because you break out of the loop. Otherwise you'll do ++it on an invalid iterator. Oh, and you haven't actually swapped the two lines in patch set 7.
Thank you for the very detailed explanation. >Oh, and you haven't actually swapped the two lines in patch set 7. Patch set #7 was missing some of the other changes as well. I believe they are now corrected in path set #8.
lgtm
Bo, now that there is an LGTM, could you please land this changelist?
On 2014/12/17 21:55:42, jab wrote: > Bo, now that there is an LGTM, could you please land this changelist? Thanks Jeff. I have committed and you can close this issue manually. Seems an empty line is needed before the "one line commit title" in the description, otherwise everything is shown in the log. Good to know for next time.
Message was sent while issue was closed.
On 2014/12/18 01:55:17, Bo Xu wrote: > On 2014/12/17 21:55:42, jab wrote: > > Bo, now that there is an LGTM, could you please land this changelist? > > Thanks Jeff. I have committed and you can close this issue manually. > > Seems an empty line is needed before the "one line commit title" in the > description, otherwise everything is shown in the log. Good to know for next > time. committed at: https://pdfium.googlesource.com/pdfium/+/bf42dfea544a8ea3269b139e940f3f8eb38f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
