bpo-35196: optimize Squeezer's write() interception by taleinat · Pull Request #10454 · python/cpython

taleinat · 2018-11-10T21:43:59Z

This also cleans up and improves the testing of count_lines(), which has been significantly changed as part of this optimization.

https://bugs.python.org/issue35196

terryjreedy · 2018-12-24T01:36:44Z

After partial first review:

Squeezer only applies to Shell, not editor or output windows. So it seems that ii should only be imported into pyshell and the handler only bound in PyShell. This itself could be fixed in another issue. However, some of the existing and proposed code

The Squeezer.init check "if isinstance(editwin, PyShell):" seems unneeded as the event handler function can only be called from the Shell write() and context menu.

The statement "Tab width is configurable" in Squeezer.count_lines is not true in IDLE and especially not in Shell. Configdialog lines 8 and 9, written in 2003 and 2005, say
"Note that tab width in IDLE is currently fixed at eight due to Tk issues.
Refer to comments in EditorWindow autoindent code for details."
I did not find anything about Tk issues in the autoindent code following editor line 1208. The comment may be obsolete, but since automatically expands user typed tabs, the point is mostly. So I think the count_lines_with_wrapping tabwidth parameter could be replaced with a local assignment.
tabwidth = 8
and all the testing involving a 'settable' tabwidth removed.

The linewidth stuff is ugly. Tk initially calculates pixel width from configured width in characters, (average) font character width, padding. Too bad it apparently does not itself do the reverse calculaton need for squeezer.

Lib/idlelib/squeezer.py

bedevere-bot · 2018-12-24T02:26:05Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

taleinat · 2018-12-24T08:05:59Z

@terryjreedy, following your comment on the subject, I did some timings with and without the second short-circuit check and found that it had no significant effect even in the most extreme cases it was meant to optimize. I've removed it to avoid the unnecessary complexity.

This is cleaner and allows simplifying Squeezer.reload().

taleinat · 2018-12-24T12:28:39Z

@terryjreedy

So I think the count_lines_with_wrapping() tabwidth parameter could be replaced with a local assignment, tabwidth = 8, and all the testing involving a 'settable' tabwidth removed.

Done.

I have made the requested changes; please review again.

bedevere-bot · 2018-12-24T12:28:41Z

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

terryjreedy · 2018-12-27T22:39:10Z

I am reviewing this and making a few changes now.

taleinat · 2019-01-12T17:31:22Z

That assertion still fails on AppVeyor: zero_char_width is 8 before and after the reload() call. This is despite the test requiring the "gui" resource. See traceback below.

What should we do with this?

Test the reload() class-method.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\cpython\lib\idlelib\idle_test\test_squeezer.py", line 321, in test_reload
    self.assertGreater(squeezer.zero_char_width, orig_zero_char_width)
AssertionError: 8 not greater than 8```

taleinat · 2019-01-12T17:33:46Z

That assertion still fails on AppVeyor

It passes locally on Windows 10, by the way.

taleinat · 2019-01-12T17:35:51Z

That assertion still fails on AppVeyor

It fails on Travis-CI too :(

Do we just drop this assertion?

terryjreedy · 2019-01-12T18:16:08Z

Cheryl's Ubuntu off-by-1 rounding failure and the CI no-change failures had different causes. I am experimenting with her suggested changes that avoid idleConf and expect to push something for the CI bots.

Whoops, hit wrong comment button.

terryjreedy · 2019-01-12T19:12:36Z

Lib/idlelib/squeezer.py

+    def load_font(self):
+        text = self.base_text
+        self.zero_char_width = \
+            Font(text, name=text.cget('font')).measure('0')


Bug. The font name is a string added to tk's registry of fonts. The tuple gets turned into a string. Since I cannot accept a suggestion after making myself assignee (contrary to (i) information popup, I will change 'name' to 'font' in the web editor and see if it changes CI bots. (Test still pass on my machine.)

terryjreedy · 2019-01-12T23:13:26Z

Thank you Cheryl, your suggestion works for the CI bots. After experimentation, I expect the new code should work even if a system does not have 'Courier'. On my machine, an unknown family is replaced by a default system font (Ariel, for me) and I assume that this is universal tk behavior.

Tal, go ahead and merge.

miss-islington · 2019-01-13T15:01:53Z

Thanks @taleinat for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

bedevere-bot · 2019-01-13T15:02:27Z

GH-11541 is a backport of this pull request to the 3.7 branch.

The new functionality of Squeezer.reload() is also tested, along with some general re-working of the tests in test_squeezer.py. (cherry picked from commit 39a33e9) Co-authored-by: Tal Einat <taleinat+github@gmail.com>

bedevere-bot · 2019-01-13T15:24:16Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Installed with X 3.x has failed when building commit 39a33e9.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/103/builds/1964) and take a look at the build logs.
Check if the failure is related to this commit (39a33e9) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/103/builds/1964

Click to see traceback logs

From https://github.com/python/cpython
 * branch                  master     -> FETCH_HEAD
Reset branch 'master'

Python/pystate.c: In function ‘_long_shared’:
Python/pystate.c:1483:18: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     data->data = (void *)value;
                  ^

/buildbot/tmp/tmpdir/tmplub9mdym/pip-18.1-py2.py3-none-any.whl/pip/_vendor/requests/status_codes.py:3: SyntaxWarning: invalid escape sequence \o
/buildbot/tmp/tmpdir/tmplub9mdym/pip-18.1-py2.py3-none-any.whl/pip/_vendor/requests/status_codes.py:3: SyntaxWarning: invalid escape sequence \o

bedevere-bot · 2019-01-13T15:25:10Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Non-Debug with X 3.x has failed when building commit 39a33e9.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/99/builds/2024) and take a look at the build logs.
Check if the failure is related to this commit (39a33e9) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/99/builds/2024

Click to see traceback logs

Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.nondebug/build/Lib/idlelib/idle_test/test_squeezer.py", line 310, in test_reload
    self.assertGreater(squeezer.zero_char_width, orig_zero_char_width)
AssertionError: 6 not greater than 6

----------------------------------------------------------------------

Ran 475 tests in 8.425s

FAILED (failures=1, skipped=3)


Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.nondebug/build/Lib/idlelib/idle_test/test_squeezer.py", line 310, in test_reload
    self.assertGreater(squeezer.zero_char_width, orig_zero_char_width)
AssertionError: 6 not greater than 6

----------------------------------------------------------------------

Ran 475 tests in 5.916s

FAILED (failures=1, skipped=3)

taleinat · 2019-01-13T15:39:54Z

The problematic test_reload() is failing on the Gentoo buildbots:

======================================================================
FAIL: test_reload (idlelib.idle_test.test_squeezer.SqueezerTest)
Test the reload() class-method.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.nondebug/build/Lib/idlelib/idle_test/test_squeezer.py", line 310, in test_reload
    self.assertGreater(squeezer.zero_char_width, orig_zero_char_width)
AssertionError: 6 not greater than 6
----------------------------------------------------------------------``

terryjreedy · 2019-01-13T16:36:02Z

I will open a new issue and PR to fix this, initially by disabling it. I approved the backport since I will follow with the initial fix and I want 3.7 in sync with master.

The new functionality of Squeezer.reload() is also tested, along with some general re-working of the tests in test_squeezer.py. (cherry picked from commit 39a33e9) Co-authored-by: Tal Einat <taleinat+github@gmail.com>

terryjreedy · 2019-01-13T17:42:51Z

Issue 35730, PR #11543.

taleinat added needs backport to 3.6 performance Performance or resource usage skip news labels Nov 10, 2018

taleinat requested a review from terryjreedy November 10, 2018 21:44

the-knights-who-say-ni added the CLA signed label Nov 10, 2018

bedevere-bot added the awaiting merge label Nov 10, 2018

terryjreedy removed the needs backport to 3.6 label Dec 11, 2018

terryjreedy removed the skip news label Dec 23, 2018

terryjreedy requested changes Dec 24, 2018

View reviewed changes

Lib/idlelib/squeezer.py Outdated Show resolved Hide resolved

Lib/idlelib/squeezer.py Outdated Show resolved Hide resolved

Lib/idlelib/squeezer.py Outdated Show resolved Hide resolved

Lib/idlelib/squeezer.py Outdated Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting merge labels Dec 24, 2018

taleinat force-pushed the bpo-35196/optimize_squeezer_write branch from 811214a to b77de8c Compare December 24, 2018 08:03

taleinat added the skip news label Dec 24, 2018

taleinat added 6 commits December 24, 2018 14:25

bpo-35196: optimize Squeezer's write() interception

2525992

remove dead, old test code from test_squeezer.py

6b60df6

bpo-35196: fix test_reload() failing

0e30a72

remove unnecessary additional check which doesn't improve performance

76e8ea2

instantiate Squeezer only on PyShell windows

65ee056

This is cleaner and allows simplifying Squeezer.reload().

hard-code tab width to 8 (always true for IDLE's shell)

485d954

taleinat force-pushed the bpo-35196/optimize_squeezer_write branch from 9275b43 to 485d954 Compare December 24, 2018 12:27

bedevere-bot removed the awaiting changes label Dec 24, 2018

bedevere-bot added the awaiting change review label Dec 24, 2018

Merge remote-tracking branch 'origin/master' into pr_10454

22f1c4f

terryjreedy closed this Jan 12, 2019

terryjreedy reopened this Jan 12, 2019

terryjreedy reviewed Jan 12, 2019

View reviewed changes

terryjreedy self-assigned this Jan 12, 2019

terryjreedy added 2 commits January 12, 2019 14:22

Change 'name' to 'font' in load_font Font().

05543b0

Reset font size by resetting font option.

66c01e5

terryjreedy approved these changes Jan 12, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 12, 2019

terryjreedy removed the skip news label Jan 12, 2019

minor code cleanup in Squeezer's mywrite()

ae8a483

taleinat merged commit 39a33e9 into python:master Jan 13, 2019

bedevere-bot removed the awaiting merge label Jan 13, 2019

bedevere-bot removed the needs backport to 3.7 label Jan 13, 2019

taleinat deleted the bpo-35196/optimize_squeezer_write branch January 14, 2019 10:13

Uh oh!

Conversation

taleinat commented Nov 10, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terryjreedy commented Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Dec 24, 2018

Uh oh!

taleinat commented Dec 24, 2018

Uh oh!

taleinat commented Dec 24, 2018

Uh oh!

bedevere-bot commented Dec 24, 2018

Uh oh!

terryjreedy commented Dec 27, 2018

Uh oh!

taleinat commented Jan 12, 2019

Uh oh!

taleinat commented Jan 12, 2019

Uh oh!

taleinat commented Jan 12, 2019

Uh oh!

terryjreedy commented Jan 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terryjreedy Jan 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy commented Jan 12, 2019

Uh oh!

miss-islington commented Jan 13, 2019

Uh oh!

bedevere-bot commented Jan 13, 2019

Uh oh!

bedevere-bot commented Jan 13, 2019

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

bedevere-bot commented Jan 13, 2019

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

taleinat commented Jan 13, 2019

Uh oh!

terryjreedy commented Jan 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terryjreedy commented Jan 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

taleinat commented Nov 10, 2018 •

edited by bedevere-bot

Loading

terryjreedy commented Dec 24, 2018 •

edited

Loading

terryjreedy commented Jan 12, 2019 •

edited

Loading

terryjreedy Jan 12, 2019 •

edited

Loading

terryjreedy commented Jan 13, 2019 •

edited

Loading