Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-30416: Protect the optimizer during constant folding. #4860

Merged
merged 5 commits into from Dec 15, 2017

Conversation

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Dec 14, 2017

It no longer spends much time doing complex calculations and no longer consumes much memory for creating large constants that will be dropped later.

This fixes also bpo-21074.

https://bugs.python.org/issue30416

It no longer spends much time doing complex calculations and no
longer consumes much memory for creating large constants that will
be dropped later.

This fixes also bpo-21074.
Copy link
Member

methane left a comment

LGTM

@serhiy-storchaka serhiy-storchaka requested a review from rhettinger Dec 14, 2017
Copy link
Member

vstinner left a comment

Thank you @serhiy-storchaka for working on this! A few comments.

return limit;
}

#define MAX_INT_SIZE 128

This comment has been minimized.

Copy link
@vstinner

vstinner Dec 14, 2017

Member

Would you mind to specify the unit? (bytes? bits? digits?)

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Dec 14, 2017

Author Member

Bits.


#define MAX_INT_SIZE 128
#define MAX_COLLECTION_SIZE 256
#define MAX_STR_SIZE 4096

This comment has been minimized.

Copy link
@vstinner

vstinner Dec 14, 2017

Member

Ditto: number of characters?

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Dec 14, 2017

Author Member

Characters.

#define MAX_INT_SIZE 128
#define MAX_COLLECTION_SIZE 256
#define MAX_STR_SIZE 4096
#define MAX_TOTAL_ITEMS 1000000

This comment has been minimized.

Copy link
@vstinner

vstinner Dec 14, 2017

Member

Wow, that's a lot. What's the rationale for so big objects? The old peephole optimizer limited to tuples of 20 items or something like that, no?

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Dec 14, 2017

Author Member

The old peephole optimizer doesn't have such limit (see bpo-30416). This is a limit for the total number of items, including nested tuples and sets. For example ((((((((1,)*20,)*20,)*20,)*20,)*20,)*20,)*20,)*20 contains around 21**8 items.

The size of every tuple and set is limited to 256 (was 20).

What the limit looks reasonable to you?

This comment has been minimized.

Copy link
@vstinner

vstinner Dec 14, 2017

Member

It's a matter of tradeoff between CPU (compute at runtime) vs disk space/memory.

If the optimized tuple is in a function which is almost never called, it would be waste to allocate 1000000 in memory, whereas it's never used.

IHMO the maximum size of "a constant" (a constant could be a tuple with nested tuples) should be 4 KB.

sys.getsizeof((None,)*500)
4048

So I suggest to limit to 500 items, not 1000000.

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Dec 14, 2017

Author Member

500 is too small. A tuple of 256 pairs contains 256*3 items total. I have decreased the limit to 1024.

This comment has been minimized.

Copy link
@vstinner

vstinner Dec 14, 2017

Member

Why do you want to build a constant for a tuple of 768 items? What is the use case? Is it a performance issue to build it at runtime?

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Dec 14, 2017

Author Member

Constant tuples or strings of size 256 are used in decoders. Constant dicts of tens elements are used in encoders. In sre_compile.py there is a constant expression b'0' + b'1' * 255. I think the size 256 is the reasonable limit of the size of constant collections. If the collection contains not primary types, but tuples, the total number of items is multiple times the size of the collection.

Using too small limit can cause a regression. Note that currently there is no limit. 1024 is much lesser than infinity.

if (vbits == (size_t)-1 || wbits == (size_t)-1) {
return NULL;
}
if (vbits + wbits > MAX_INT_SIZE) {

This comment has been minimized.

Copy link
@vstinner

vstinner Dec 14, 2017

Member

There is a risk of integer overflow, no? May check "vbits+wbits < SIZE_MAX" with "vbits < SIZE_MAX - wbits" ?

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Dec 14, 2017

Ok for 1024 items, but why limiting a single tuple to 256? Maybe 1024 should be used for all limits?

@serhiy-storchaka

This comment has been minimized.

Copy link
Member Author

serhiy-storchaka commented Dec 14, 2017

Oh, it already is increased from 20 to 256. I think this is enough.

@serhiy-storchaka serhiy-storchaka merged commit 2e3f570 into python:master Dec 15, 2017
4 checks passed
4 checks passed
bedevere/issue-number Issue number 30416 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@serhiy-storchaka serhiy-storchaka deleted the serhiy-storchaka:safe-const-folding branch Dec 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.