New issue
Advanced search Search tips

Issue 177 link

Starred by 8 users

Issue metadata

Status: WontFix
Owner:
Closed: Jul 25



Sign in to add a comment

CFX_FixedBufGrow seems dubious.

Project Member Reported by tsepez@chromium.org, Jun 30 2015

Issue description

As opposed to just x = new blah[size];  Yes, a lot of the time you can avoid a malloc, but the additional complexity doesn't seem worthwhile.

 
Project Member

Comment 1 by tsepez@chromium.org, Jul 26 2016

Owner: ----
Status: New (was: Accepted)
Project Member

Comment 2 by dsinclair@chromium.org, Mar 6 2017

Status: Accepted (was: New)
Project Member

Comment 3 by thestig@chromium.org, Apr 26

Do we still want to get rid of it? There are only ~25 instantiations.
Project Member

Comment 4 by dsinclair@chromium.org, Apr 26

Yea, I'd like to. I did the work to remove all of them at once and got a perf regression on Win/Mac. We need to go through and remove them one at a time and figure out which ones really care about the 16 item buffer that's pre-allocated.

Original CL: https://pdfium-review.googlesource.com/c/pdfium/+/3138
Revert: https://pdfium-review.googlesource.com/c/pdfium/+/3158
Project Member

Comment 5 by npm@chromium.org, May 4

Owner: npm@chromium.org
I will try to get rid of this, maybe trying to avoid vector since it appears to be smaller for some of these use cases.
Project Member

Comment 6 by tsepez@chromium.org, May 4

It looks like most of the remaining uses are associated with places where there are an unbounded number of color components.  Unless we're willing to put a hard limit on these, and perhaps break some documents, this is serving a purpose. Performance gets poor when we've removed this in the past.

I think we can wontfix this one given the reduced scope.  Second?
Project Member

Comment 7 by dsinclair@chromium.org, May 7

I'd like to know why the original patch caused a slowdown on windows/mac before closing this. Can we change a bunch of these and leave one or two? Was it one specific change that caused the regression? Can we move FixedBufGrow to be anonymous in the one place it's needed?

Project Member

Comment 8 by npm@chromium.org, May 7

Well the first question is whether we replace with the fixed size array or the buffer that supports large sizes (unique_ptr/vector/etc). Arrays are fast but vector support big sizes...

I had a CL to start doing a per file removal https://pdfium-review.googlesource.com/c/pdfium/+/32071 (at least one of those shouldn't be removed), but I think Tom would prefer to replace with fixed size array. This would crash when we need a large sized array.

If we change to vector, I'm not sure our performance tests will be able to determine if we'll cause perf regressions in some PDFs in the wild. We'd need to trust our instincts on this I guess.
Project Member

Comment 9 by dsinclair@chromium.org, May 7

Changing to vector is what I did in comment #4 and we detected the regression. I don't think replacing all of them at once was the right solution, I think we need to each one in it's own CL which we then run the pref test against.

My original CL went with a vector sized as needed, but I don't know what caused the actual slowdown there, was it vector allocation, did I forget to resize() or something else?
Project Member

Comment 10 by npm@chromium.org, May 7

I think the regression was that the majority of CFX_FixedBufGrow used is for small sizes, and std::vector has debug checks that make it slow. Another possible reason for it becoming slower was creating a vector of the size immediately (causing it to be 0-initialized) instead of creating an empty vector and then reserving the memory.
Project Member

Comment 11 by dsinclair@chromium.org, May 7

Those would both be good things to verify:

 1. Do we only regress on Debug (this would be acceptable if Release does not regress)
 2. Does reserving() instead of pre-allocating fix the regression.
Project Member

Comment 12 by npm@chromium.org, May 7

Ok I'll run corpus tests locally and see if I find any useful data.
Project Member

Comment 13 by npm@chromium.org, May 7

After running the safetynet_compare.py on Linux Debug comparing master vs Dan's CL using vectors I get only 2 regressions (values in %):
+16.66 testing/corpus/fx/FRC_4.5_part1/FRC_4.5.5_Pattern_shading.pdf
+1369.83 testing/corpus/pdfium/bug_583804.pdf

On Release I get the same regressions, but different percentages:
+21.4 testing/corpus/fx/FRC_4.5_part1/FRC_4.5.5_Pattern_shading.pdf
+172.25 testing/corpus/pdfium/bug_583804.pdf

If I reinstate the CFX_FixedBufGrow usage again on CPDF_DIBSource::DownSampleScanline32Bit and CPDF_SampledFunc::v_Call, I get 0 regressions on Debug. So in terms of our performance testing we only need two of the usages.
Project Member

Comment 14 by dsinclair@chromium.org, May 7

Sweet, so lets the all of them in except for those two and then we can investigate if there is a way to clean up those two usages.

Thanks a lot for investigating.
Project Member

Comment 15 by bugdroid1@chromium.org, May 8

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/5de481e71bcde25d31452b23a017bb783163a204

commit 5de481e71bcde25d31452b23a017bb783163a204
Author: Nicolas Pena <npm@chromium.org>
Date: Tue May 08 19:13:28 2018

Remove almost all usages of CFX_FixedBufGrow with std::vector

Tested by running safetynet_compare.py on this patch vs master. The
results were 0 regressions and 0 improvements. The two remaining usages
cannot be replaced because they would cause a regression.

Bug:  pdfium:177 
Change-Id: I43eddf4ffaac2eb063f2004d6606bc3cd6e627ac
Reviewed-on: https://pdfium-review.googlesource.com/32159
Reviewed-by: dsinclair <dsinclair@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>

[modify] https://crrev.com/5de481e71bcde25d31452b23a017bb783163a204/core/fpdfapi/page/cpdf_colorspace.cpp
[modify] https://crrev.com/5de481e71bcde25d31452b23a017bb783163a204/core/fpdfapi/render/cpdf_dibsource.cpp
[modify] https://crrev.com/5de481e71bcde25d31452b23a017bb783163a204/core/fxge/apple/fx_apple_platform.cpp
[modify] https://crrev.com/5de481e71bcde25d31452b23a017bb783163a204/core/fpdfapi/render/cpdf_renderstatus.cpp
[modify] https://crrev.com/5de481e71bcde25d31452b23a017bb783163a204/core/fxcodec/codec/fx_codec_icc.cpp

Project Member

Comment 16 by bugdroid1@chromium.org, Jun 13

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/fb6b382bea56ca22bacce1379230680d17cf5896

commit fb6b382bea56ca22bacce1379230680d17cf5896
Author: Nicolas Pena <npm@chromium.org>
Date: Wed Jun 13 18:15:47 2018

Remove CFX_FixedBufGrow from fx_apple_platform.cpp

Bug:  pdfium:177 
Change-Id: I63f2e7579de37f52fd67f02988d72de7e1b3c7ba
Reviewed-on: https://pdfium-review.googlesource.com/35111
Commit-Queue: dsinclair <dsinclair@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/fb6b382bea56ca22bacce1379230680d17cf5896/core/fxge/apple/fx_apple_platform.cpp

Project Member

Comment 17 by bugdroid1@chromium.org, Jun 13

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/ae7bcb7c9ec3a617a464697845968401391f74c9

commit ae7bcb7c9ec3a617a464697845968401391f74c9
Author: Nicolas Pena <npm@chromium.org>
Date: Wed Jun 13 18:54:06 2018

Remove CFX_FixedBufGrow from fx_codec_icc.cpp

Bug:  pdfium:177 
Change-Id: Ib4de4f258ebd98a53b309c30b7e4aa28f0c581eb
Reviewed-on: https://pdfium-review.googlesource.com/35112
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/ae7bcb7c9ec3a617a464697845968401391f74c9/core/fxcodec/codec/fx_codec_icc.cpp

Project Member

Comment 18 by bugdroid1@chromium.org, Jun 13

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/07d52a916ddc6d7e04845598376e033f2a04faba

commit 07d52a916ddc6d7e04845598376e033f2a04faba
Author: Nicolas Pena <npm@chromium.org>
Date: Wed Jun 13 20:33:47 2018

Remove almost all usages of CFX_FixedBufGrow from cpdf_dibsource.cpp

This CL removes all the usages of CFX_FixedBufGrow, except for one that would
cause performance issues for our corpus tests.

Bug:  pdfium:177 
Change-Id: I0ad76c14f713b116cf7dce50606554e3b03d9f2c
Reviewed-on: https://pdfium-review.googlesource.com/35150
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/07d52a916ddc6d7e04845598376e033f2a04faba/core/fpdfapi/render/cpdf_dibsource.cpp

Project Member

Comment 19 by bugdroid1@chromium.org, Jun 13

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/fb73bcdb998550bd537fb5e735dc71198e80c4d5

commit fb73bcdb998550bd537fb5e735dc71198e80c4d5
Author: Nicolas Pena <npm@chromium.org>
Date: Wed Jun 13 20:41:27 2018

Remove CFX_FixedBufGrow from cpdf_colorspace.cpp

Bug:  pdfium:177 
Change-Id: I92e71fd0f2445736680e1cf9e7cc41bda8e6505e
Reviewed-on: https://pdfium-review.googlesource.com/35114
Reviewed-by: dsinclair <dsinclair@chromium.org>
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>

[modify] https://crrev.com/fb73bcdb998550bd537fb5e735dc71198e80c4d5/core/fpdfapi/page/cpdf_colorspace.cpp

Project Member

Comment 20 by bugdroid1@chromium.org, Jun 13

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/ff94c9e01e02cb7c658eaef4641f57d5681553a5

commit ff94c9e01e02cb7c658eaef4641f57d5681553a5
Author: Nicolas Pena <npm@chromium.org>
Date: Wed Jun 13 20:43:47 2018

Remove CFX_FixedBufGrow from cpdf_renderstatus.cpp

Bug:  pdfium:177 
Change-Id: I58fe339d5a0a962215c5cb29c963b37b86832637
Reviewed-on: https://pdfium-review.googlesource.com/35113
Commit-Queue: dsinclair <dsinclair@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/ff94c9e01e02cb7c658eaef4641f57d5681553a5/core/fpdfapi/render/cpdf_renderstatus.cpp

Project Member

Comment 21 by npm@chromium.org, Jul 25

Status: WontFix (was: Accepted)
I'm closing this now as the remaining use cases seem to be needed for performance reasons, according to our tests.
Project Member

Comment 22 by thestig@chromium.org, Sep 20

 Issue 758  has been merged into this issue.

Sign in to add a comment