Various changes to JBig2 cache:

Various changes to JBig2 cache:
- Makes the cache be per-document
- Keys the cache on ObjNum and stream offset instead of keying on a pointer to the data (which can result in false cache hits).
- Makes it so the cache is only used for the globals stream.
- Reenable the cache.

R=thestig@chromium.org

BUG=pdfium:207
Committed: https://pdfium.googlesource.com/pdfium/+/f1b88e76134808f36f16b9e53a2e9dd89b12c8fd

[David Lattimore, 2015-10-06 02:30:45]

I ran with and without this change and checked the number of cache hits on
experiment.pdf. These were the results:
without this change:
  cache size=2: 165 hits
  cache size=1: 0 hits
with this change:
  cache size=2: 165 hits
  cache size=1: 165 hits

So we can probably reduce the cache size. That said, I'm also happy to keep it
at 2, possibly removing the changes to only use the cache on the globals dict.

[jbreiden, 2015-10-06 03:30:58]

This sounds a little suspicious, because the 'Digitized by Google' 
watermark is also JBIG2 but uses a separate symbol dictionary from 
everything else. Suggest wwo quick sanity checks.

1. Count calls to CJBig2_Context::parseSymbolDict(). I am expecting
that to be 2 for cache size 2.

2. Confirm the cache is dramatically reducing cpu time (I reported 39 
to 14 seconds in bug pdfium:85)

3. If everything looks awesome with cache size 1 and we can't explain 
why, I still think it should be 2. Not sure I can articulate reasoning.

[David Lattimore, 2015-10-06 05:20:09]

I see 332 calls to CJBig2_Context::parseSymbolDict() regardless of whether the
cache is enabled (both without this change). Did you mean some other function?

[jbreiden, 2015-10-06 21:58:29]

Yes, my mistake. I just want to positively confirm that we aren't 
making lots of unnecessary, expensive calls through some other 
unexpected code path. The expensive call is either of these:

CJBig2_SDDProc::decode_Arith
CJBig2_SDDProc::decode_Huffman

[Lei Zhang, 2015-10-06 22:40:24]

I'll leave it to you and Jeff to figure out if the cache is effective.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/codec/code...
File core/src/fxcodec/codec/codec_int.h (left):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/codec/code...
core/src/fxcodec/codec/codec_int.h:283: FX_BOOL m_bFileReader;
If you want to spin this off into its own separate CL, I can easily review it.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/codec/fx_c...
File core/src/fxcodec/codec/fx_codec_jbig.cpp (right):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_jbig.cpp:29: void* pModulePrivateData =
pPrivateData->GetPrivateData(pModule);
It's silly that GetPrvigateData() does not return a CFX_DestructObject*, but we
can fix that later.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_BitStream.h (right):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_BitStream.h:15: CJBig2_BitStream(CPDF_StreamAcc*
pSrcStream);
add the explicit keyword

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_BitStream.h:69: FX_DWORD getObjNum();
This can be a const method.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_BitStream.h:80: FX_DWORD m_dwObjNum;
If you move the initialization in the ctors to the initializer list, you can
make this const.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_BitStream.h:82: inline
CJBig2_BitStream::CJBig2_BitStream(CPDF_StreamAcc* pSrcStream) {
Oh geez, I need to fix more of these silly inlining later.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:26: // Disabled until we can figure out
how to clear cache between documents.
This should be deleted too.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:28: #ifndef DISABLE_SYMBOL_CACHE
Do we still want to keep this then? With a proper fix, I see no real need to
offer the option to disable the cache.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:626: for
(std::list<CJBig2_CachePair>::iterator it = m_pSymbolDictCache->begin();
You can probaby use auto here.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.h (right):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.h:23: using CJBig2_CacheKey =
std::pair<FX_DWORD, FX_DWORD>;
Can you document what the key / value is?

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.h:24: using CJBig2_CachePair =
std::pair<CJBig2_CacheKey, CJBig2_SymbolDict*>;
CJBig2_CachePair owns the CJBig2_SymbolDict* right? It would be good to mention
that.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.h:61: bool isGlobal);
bIsGlobal if you want to be consistent with the rest of the code that uses
Hungarian notation.

[jbreiden, 2015-10-07 17:50:16]

jbreiden@google.com changed reviewers:
+ jbreiden@google.com

[jbreiden, 2015-10-07 17:50:17]

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:21: // again. We key off of the memory
location of the dictionary. The
not keying off memory location anymore.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:28: #ifndef DISABLE_SYMBOL_CACHE
I'd like to think that we can disable the cache any time we want just by setting
kSymbolDictCacheMaxSize to 0. No #ifdef required. I suppose it makes sense to
try that before checking in to make sure nothing crashes.

Also, I think this should be 2 because:
 
  (a) I still can't wrap my head around why we can get away with 1.
  (b) 2 exercises the LRU machinery hopefully keeping it from bitrot.
  (c) I don't see much downside

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:659: #ifndef DISABLE_SYMBOL_CACHE
not sure why we have the ifdef

[Lei Zhang, 2015-10-07 17:59:24]

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:28: #ifndef DISABLE_SYMBOL_CACHE
On 2015/10/07 17:50:17, jbreiden wrote:
> I'd like to think that we can disable the cache any time we want just by
setting
> kSymbolDictCacheMaxSize to 0. No #ifdef required. I suppose it makes sense to
> try that before checking in to make sure nothing crashes.

No, you can't. Setting it to 0 triggers compiler warnings with Clang, and we
want to get to a world where (core) PDFium is warning free.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:659: #ifndef DISABLE_SYMBOL_CACHE
On 2015/10/07 17:50:17, jbreiden wrote:
> not sure why we have the ifdef

Because the compiler sees a constant with a value 0 and decides some of the if /
while statements below are worthless.

[jbreiden, 2015-10-07 19:22:51]

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:659: #ifndef DISABLE_SYMBOL_CACHE
Something seems wrong when a compiler pushes us to use an ifdef in the name of
code cleanliness. 

If kSymbolDictCacheMaxSize can never be 0, there's not a lot of benefit from the
(kSymbolDictCacheMaxSize > 0) check below. But honestly, this probably is not
worth sweating over one way or the other.

[Lei Zhang, 2015-10-07 19:31:59]

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:659: #ifndef DISABLE_SYMBOL_CACHE
On 2015/10/07 19:22:51, jbreiden wrote:
> Something seems wrong when a compiler pushes us to use an ifdef in the name of
> code cleanliness. 
> 
> If kSymbolDictCacheMaxSize can never be 0, there's not a lot of benefit from
the
> (kSymbolDictCacheMaxSize > 0) check below. But honestly, this probably is not
> worth sweating over one way or the other.
> 

I agree Clang behaved unexpectedly in this case. It did not warn about the if
statement, which always evals to false, but did warn about the while statement
that always evals to true.

But taking a step back, this is silly to argue about. Let's just fix the bug and
get rid of DISABLE_SYMBOL_CACHE and call it a day.

[David Lattimore, 2015-10-07 22:41:53]

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/codec/code...
File core/src/fxcodec/codec/codec_int.h (left):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/codec/code...
core/src/fxcodec/codec/codec_int.h:283: FX_BOOL m_bFileReader;
On 2015/10/06 22:40:23, Lei Zhang wrote:
> If you want to spin this off into its own separate CL, I can easily review it.

Done.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/codec/fx_c...
File core/src/fxcodec/codec/fx_codec_jbig.cpp (right):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_jbig.cpp:29: void* pModulePrivateData =
pPrivateData->GetPrivateData(pModule);
On 2015/10/06 22:40:24, Lei Zhang wrote:
> It's silly that GetPrvigateData() does not return a CFX_DestructObject*, but
we
> can fix that later.

I think it's only guaranteed to be CFX_DestructObject* if SetPrivateObj was
called. If SetPrivateData was called instead, it could be anything.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_BitStream.h (right):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_BitStream.h:15: CJBig2_BitStream(CPDF_StreamAcc*
pSrcStream);
On 2015/10/06 22:40:24, Lei Zhang wrote:
> add the explicit keyword

Done.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_BitStream.h:69: FX_DWORD getObjNum();
On 2015/10/06 22:40:24, Lei Zhang wrote:
> This can be a const method.

Done.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_BitStream.h:80: FX_DWORD m_dwObjNum;
On 2015/10/06 22:40:24, Lei Zhang wrote:
> If you move the initialization in the ctors to the initializer list, you can
> make this const.

Done.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:26: // Disabled until we can figure out
how to clear cache between documents.
On 2015/10/06 22:40:24, Lei Zhang wrote:
> This should be deleted too.

Done.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:28: #ifndef DISABLE_SYMBOL_CACHE
Removed DISABLE_SYMBOL_CACHE

Set cache size back to 2. I agree there's not much downside. Especially now the
cache is owned by the document so will be freed at the end of the document.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:626: for
(std::list<CJBig2_CachePair>::iterator it = m_pSymbolDictCache->begin();
On 2015/10/06 22:40:24, Lei Zhang wrote:
> You can probaby use auto here.

Done.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:659: #ifndef DISABLE_SYMBOL_CACHE
On 2015/10/07 19:31:59, Lei Zhang wrote:
> On 2015/10/07 19:22:51, jbreiden wrote:
> > Something seems wrong when a compiler pushes us to use an ifdef in the name
of
> > code cleanliness. 
> > 
> > If kSymbolDictCacheMaxSize can never be 0, there's not a lot of benefit from
> the
> > (kSymbolDictCacheMaxSize > 0) check below. But honestly, this probably is
not
> > worth sweating over one way or the other.
> > 
> 
> I agree Clang behaved unexpectedly in this case. It did not warn about the if
> statement, which always evals to false, but did warn about the while statement
> that always evals to true.
> 
> But taking a step back, this is silly to argue about. Let's just fix the bug
and
> get rid of DISABLE_SYMBOL_CACHE and call it a day.

Done.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.h (right):

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.h:23: using CJBig2_CacheKey =
std::pair<FX_DWORD, FX_DWORD>;
On 2015/10/06 22:40:24, Lei Zhang wrote:
> Can you document what the key / value is?

Done.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.h:24: using CJBig2_CachePair =
std::pair<CJBig2_CacheKey, CJBig2_SymbolDict*>;
On 2015/10/06 22:40:24, Lei Zhang wrote:
> CJBig2_CachePair owns the CJBig2_SymbolDict* right? It would be good to
mention
> that.

Done.

https://codereview.chromium.org/1380243004/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.h:61: bool isGlobal);
On 2015/10/06 22:40:24, Lei Zhang wrote:
> bIsGlobal if you want to be consistent with the rest of the code that uses
> Hungarian notation.

Done.

[Lei Zhang, 2015-10-08 03:07:53]

Will look. Thanks for dealing with the merge conflicts.

[Lei Zhang, 2015-10-08 03:22:44]

https://codereview.chromium.org/1380243004/diff/50001/core/include/fxcodec/fx...
File core/include/fxcodec/fx_codec.h (right):

https://codereview.chromium.org/1380243004/diff/50001/core/include/fxcodec/fx...
core/include/fxcodec/fx_codec.h:13: #include "../fpdfapi/fpdf_objects.h"
nit: forward declare CFX_PrivateData and CPDF_StreamAcc instead?

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_BitStream.cpp (right):

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_BitStream.cpp:16:
m_dwObjNum(pSrcStream->GetStream()->GetObjNum()) {
Do we need to handle GetStream() returning NULL? I think 0 is the sentinel value
for bad object numbers?

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_BitStream.h (right):

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_BitStream.h:10: #include
"../../../include/fpdfapi/fpdf_objects.h"
ditto, forward declare instead.

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_BitStream.h:41: FX_DWORD getObjNum() const;
nit: this method looks lonely sitting by itself.

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_Context.cpp:69: new CJBig2_Context(NULL,
pGlobalStream, pSymbolDictCache, pPause, true);
nit: nullptr

[Lei Zhang, 2015-10-08 03:44:39]

I'm doing a Release build with Chromium's Clang as the compiler.

For control.pdf and experiment.pdf, I got 1.2s / 1.1s with the old cache
enabled. With the caching disabled, I get 8.8s / 21.6s. With this CL, it's at
6.8s / 6.8s.

[David Lattimore, 2015-10-08 03:44:39]

https://codereview.chromium.org/1380243004/diff/50001/core/include/fxcodec/fx...
File core/include/fxcodec/fx_codec.h (right):

https://codereview.chromium.org/1380243004/diff/50001/core/include/fxcodec/fx...
core/include/fxcodec/fx_codec.h:13: #include "../fpdfapi/fpdf_objects.h"
On 2015/10/08 03:22:44, Lei Zhang wrote:
> nit: forward declare CFX_PrivateData and CPDF_StreamAcc instead?

Done.

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_BitStream.cpp (right):

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_BitStream.cpp:16:
m_dwObjNum(pSrcStream->GetStream()->GetObjNum()) {
On 2015/10/08 03:22:44, Lei Zhang wrote:
> Do we need to handle GetStream() returning NULL? I think 0 is the sentinel
value
> for bad object numbers?

I'm not sure. To err on the side of caution, I put a null check in. It would
probably be better if there were a global constant kBadObjNum or similar rather
than an explicit 0, but I wasn't sure where to look for such a constant.

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_BitStream.h (right):

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_BitStream.h:10: #include
"../../../include/fpdfapi/fpdf_objects.h"
On 2015/10/08 03:22:44, Lei Zhang wrote:
> ditto, forward declare instead.

Done.

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_BitStream.h:41: FX_DWORD getObjNum() const;
On 2015/10/08 03:22:44, Lei Zhang wrote:
> nit: this method looks lonely sitting by itself.

Done.

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_Context.cpp:69: new CJBig2_Context(NULL,
pGlobalStream, pSymbolDictCache, pPause, true);
On 2015/10/08 03:22:44, Lei Zhang wrote:
> nit: nullptr

Done.

[David Lattimore, 2015-10-08 03:55:07]

On 2015/10/08 03:44:39, Lei Zhang wrote:
> I'm doing a Release build with Chromium's Clang as the compiler.
> 
> For control.pdf and experiment.pdf, I got 1.2s / 1.1s with the old cache
> enabled. With the caching disabled, I get 8.8s / 21.6s. With this CL, it's at
> 6.8s / 6.8s.

I'm having trouble replicating your results for the old cache. Here are the
numbers for my machine:
control.pdf / experiment.pdf
Old cache code: 11.7 / 11.5
Cache disabled: 13.9 / 28.4
New cache code: 11.5 / 11.5

[Lei Zhang, 2015-10-08 03:56:05]

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_BitStream.cpp (right):

https://codereview.chromium.org/1380243004/diff/50001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_BitStream.cpp:16:
m_dwObjNum(pSrcStream->GetStream()->GetObjNum()) {
On 2015/10/08 03:44:39, David Lattimore wrote:
> On 2015/10/08 03:22:44, Lei Zhang wrote:
> > Do we need to handle GetStream() returning NULL? I think 0 is the sentinel
> value
> > for bad object numbers?
> 
> I'm not sure. To err on the side of caution, I put a null check in. It would
> probably be better if there were a global constant kBadObjNum or similar
rather
> than an explicit 0, but I wasn't sure where to look for such a constant.

Well, the PDF spec says positive for object numbers, and non-negative for
generation numbers. That reads as 0 is an invalid obj number to me. How about we
don't cache when the obj number is 0?

[Lei Zhang, 2015-10-08 04:03:22]

On 2015/10/08 03:55:07, David Lattimore wrote:
> On 2015/10/08 03:44:39, Lei Zhang wrote:
> > I'm doing a Release build with Chromium's Clang as the compiler.
> > 
> > For control.pdf and experiment.pdf, I got 1.2s / 1.1s with the old cache
> > enabled. With the caching disabled, I get 8.8s / 21.6s. With this CL, it's
at
> > 6.8s / 6.8s.
> 
> I'm having trouble replicating your results for the old cache. Here are the
> numbers for my machine:
> control.pdf / experiment.pdf
> Old cache code: 11.7 / 11.5
> Cache disabled: 13.9 / 28.4
> New cache code: 11.5 / 11.5

Pretty consistent numbers for me. I'm just running: time out/Release/pdfium_test
foo.pdf
old code is commit eda27bd.
Cache disabled: 1f439a7

[Lei Zhang, 2015-10-08 04:12:51]

On 2015/10/08 04:03:22, Lei Zhang wrote:
> On 2015/10/08 03:55:07, David Lattimore wrote:
> > On 2015/10/08 03:44:39, Lei Zhang wrote:
> > > I'm doing a Release build with Chromium's Clang as the compiler.
> > > 
> > > For control.pdf and experiment.pdf, I got 1.2s / 1.1s with the old cache
> > > enabled. With the caching disabled, I get 8.8s / 21.6s. With this CL, it's
> at
> > > 6.8s / 6.8s.
> > 
> > I'm having trouble replicating your results for the old cache. Here are the
> > numbers for my machine:
> > control.pdf / experiment.pdf
> > Old cache code: 11.7 / 11.5
> > Cache disabled: 13.9 / 28.4
> > New cache code: 11.5 / 11.5
> 
> Pretty consistent numbers for me. I'm just running: time
out/Release/pdfium_test
> foo.pdf
> old code is commit eda27bd.
> Cache disabled: 1f439a7

Oh, commit ce37d73 screwed everything up.

The performance numbers are fine here.

[Lei Zhang, 2015-10-08 04:14:50]

lgtm

https://codereview.chromium.org/1380243004/diff/90001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1380243004/diff/90001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_Context.cpp:600: if (m_bIsGlobal) {
How about we add a check for key.first != 0?

[Lei Zhang, 2015-10-08 04:15:05]

Also, BUG=207

[Lei Zhang, 2015-10-08 04:15:24]

On 2015/10/08 04:15:05, Lei Zhang wrote:
> Also, BUG=207

Err, BUG=pdfium:207 is probably better.

[David Lattimore, 2015-10-08 04:34:00]

https://codereview.chromium.org/1380243004/diff/90001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1380243004/diff/90001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_Context.cpp:600: if (m_bIsGlobal) {
On 2015/10/08 04:14:50, Lei Zhang wrote:
> How about we add a check for key.first != 0?

Done.

[Lei Zhang, 2015-10-08 04:40:55]

++lgtm

[David Lattimore, 2015-10-08 21:18:35]

Committed patchset #7 (id:110001) manually as
f1b88e76134808f36f16b9e53a2e9dd89b12c8fd (presubmit successful).