◐ Shell
reader mode source ↗
Skip to content

bpo-1635741: Enable unicode_release_interned() without insure or valgrind.#21087

Closed
shihai1991 wants to merge 9 commits into
python:masterfrom
shihai1991:bpo_1635741_interned_unicode
Closed

bpo-1635741: Enable unicode_release_interned() without insure or valgrind.#21087
shihai1991 wants to merge 9 commits into
python:masterfrom
shihai1991:bpo_1635741_interned_unicode

Conversation

@shihai1991

@shihai1991 shihai1991 commented Jun 23, 2020

Copy link
Copy Markdown
Member

@shihai1991

Copy link
Copy Markdown
Member Author

After this PR, from sys.gettotalrefcount: 17647 to sys.gettotalrefcount: 14775.

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Can you please try to measure the important on performance of this change? You can use:

python3 -m pyperf command -- ./python -S -c pass

@vstinner

Copy link
Copy Markdown
Member

Can you please try to measure the important on performance of this change? You can use:

Note: Please use a release build (./configure && make).

@shihai1991

shihai1991 commented Jun 23, 2020

Copy link
Copy Markdown
Member Author

Can you please try to measure the important on performance of this change? You can use:

Note: Please use a release build (./configure && make).

Hi, victor. Got this benchmark result.

$ python3 -m pyperf command -- ./python -S -c pass
.....................
WARNING: the benchmark result may be unstable

  • the standard deviation (1.62 ms) is 11% of the mean (14.3 ms)

Try to rerun the benchmark with more runs, values and/or loops.
Run 'python3 -m pyperf system tune' command to reduce the system jitter.
Use pyperf stats, pyperf dump and pyperf hist to analyze results.
Use --quiet option to hide these warnings.

command: Mean +- std dev: 14.3 ms +- 1.6 ms

$ python3 -m pyperf command -- ./python -S -c pass --duplicate=100
.....................
WARNING: the benchmark result may be unstable

  • the standard deviation (2.33 ms) is 15% of the mean (15.1 ms)

Try to rerun the benchmark with more runs, values and/or loops.
Run 'python3 -m pyperf system tune' command to reduce the system jitter.
Use pyperf stats, pyperf dump and pyperf hist to analyze results.
Use --quiet option to hide these warnings.

command: Mean +- std dev: 15.1 ms +- 2.3 ms

@vstinner

Copy link
Copy Markdown
Member

Hi, victor. Got this benchmark result.

What did you measure? What is the reference (unmodified master branch) and what is the modified code (with your PR)?

You can store the benchmark result in a file (-o bench.json) and then compare two files (python3 -m pyperf compare_to ref.json pr.json).

@vstinner

Copy link
Copy Markdown
Member

What is the dictionary size of the interned strings at Python exit (in unicode_release_interned())?

@vstinner

Copy link
Copy Markdown
Member

Please rebase your PR on master, I modified unicodeobject.c to make singletons per interpreter.

@shihai1991

Copy link
Copy Markdown
Member Author

Hi, victor. Got this benchmark result.

What did you measure? What is the reference (unmodified master branch) and what is the modified code (with your PR)?

You can store the benchmark result in a file (-o bench.json) and then compare two files (python3 -m pyperf compare_to ref.json pr.json).

python3 is master, ./python with tihs PR. I test it again after I fix the assert problem.

@vstinner

Copy link
Copy Markdown
Member

python3 is master, ./python with tihs PR

You tested ./python is both cases. Usually, to measure the performance of a change, you need a reference (your "python3") and the modified Python (your "./python").

@shihai1991

Copy link
Copy Markdown
Member Author

python3 is master, ./python with tihs PR

You tested ./python is both cases. Usually, to measure the performance of a change, you need a reference (your "python3") and the modified Python (your "./python").

ok, Look like I confused test python3's performance with use pyperf of python3 to compare other python version's load performance.
you mean this one, right?:

python3 -m pyperf command ./python_ref  xxx -o ref.json
python3 -m pyperf command ./python_pr xxx -o pr.json
python3 -m pyperf compare_to ref.json pr.json

@vstinner

Copy link
Copy Markdown
Member
python3 -m pyperf command ./python_ref  xxx -o ref.json
python3 -m pyperf command ./python_pr xxx -o pr.json
python3 -m pyperf compare_to ref.json pr.json

Yeah, that would work :-)

Anyway, I expect no significant difference. So the most important data point is the number of items in the dictionary at exit.

@shihai1991

Copy link
Copy Markdown
Member Author
python3 -m pyperf command ./python_ref  xxx -o ref.json
python3 -m pyperf command ./python_pr xxx -o pr.json
python3 -m pyperf compare_to ref.json pr.json

Yeah, that would work :-)

Anyway, I expect no significant difference. So the most important data point is the number of items in the dictionary at exit.

Got this result ;) :
$ python3.9 -m pyperf compare_to ref.json pr.json
Benchmark hidden because not significant (1): command

@shihai1991 shihai1991 requested review from a team, 1st1, rhettinger and skrah as code owners June 25, 2020 08:48
@shihai1991

Copy link
Copy Markdown
Member Author

What is the dictionary size of the interned strings at Python exit (in unicode_release_interned())?

(gdb) p PyDict_Size(interned)
$1 = 1471

@vstinner

vstinner commented Jul 1, 2020

Copy link
Copy Markdown
Member

I wrote PR #21269 which clears interned strings in the correct order.

@vstinner

vstinner commented Jul 1, 2020

Copy link
Copy Markdown
Member

I merged my PR #21269.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants