Memoize some rules to avoid exponential time in deep nesting

[pablogsal]

Additionally, fix the script to calculate the nesting level using the
generated extension.

Closes: #37

[pablogsal]

The test are failing to download openssl for some reason... :S

[lysnikolaou]

Merging upstream master would probably solve the problem.

[pablogsal]

Merging upstream master would probably solve the problem.

Hummm, merging upstream master in this PR creates a mess. Do you mean merging upstream/master in the pegen branch?

[lysnikolaou]

Merging upstream master would probably solve the problem.

Hummm, merging upstream master in this PR creates a mess. Do you mean merging upstream/master in the pegen branch?

Yes. And then merging pegen into this branch. That's how I do it.

[lysnikolaou]

Sorry for being a bit abrupt amd not explaining things carefully. I'm on my phone.

[pablogsal]

Sorry for being a bit abrupt amd not explaining things carefully. I'm on my phone.

No problem, I know that feeling very well :)

[pablogsal]

Seems that we need to update pegen to account for python#19283

[lysnikolaou]

What about del_targets? Does something like del <'a.a'*20> parse normally?

[pablogsal]

What about del_targets? Does something like del <'a.a'*20> parse normally?

I tried with:

del a.a.a.a.a......a.a (more than 4000) and seems to parse fine. Could you double check?

[pablogsal]

What about del_targets? Does something like del <'a.a'*20> parse normally?

I tried and something like f"del {'(' * nesting_depth}x,{'),' * nesting_depth}" also has exponential time, so we need to memorize del_targets as well.

Edit: PR updated

[lysnikolaou]

LGTM.

Also, really good catch! Thanks, Pablo!

Edit: I forgot something. Before we merge this, have you measured what the memory cost is for these changes? Maybe the PEP will need updating.

[pablogsal]

Edit: I forgot something. Before we merge this, have you measured what the memory cost is for these changes?

It increased 0.62% parsing xxl.txt and 0.83% parsing ((((((((((((((((((((1)))))))))))))))))))).

[pablogsal]

Checked also with the largest python file in the standard library (Lib/test/test_socket.py) and it increased the memory 1.58%.

[pablogsal]

Are you ok with these results for landing this? :)

[lysnikolaou]

How does it compare to the current parser now? Still a little bit ahead?

[pablogsal]

How does it compare to the current parser now? Still a little bit ahead?

The speed seems untouched (maybe a bit faster due to the extra memoization) if you parse something that relies heavily on target, del_target or star_targer . It would be good if you could confirm these findings on your side when possible

[lysnikolaou]

How does it compare to the current parser now? Still a little bit ahead?

The speed seems untouched (maybe a bit faster due to the extra memoization) if you parse something that relies heavily on target, del_target or star_targer . It would be good if you could confirm these findings on your side when possible

Of course, I'm working on a bug-fix for #38 and I'll verify everything right afterwards. Shouldn't be long.

[lysnikolaou]

Oops! Sorry, accidentally pushed the wrong button.

[gvanrossum]

Thanks! I discovered that script was broken when moving the code into the cpython repo, but didn't take time to look into it.

Any chance of also committing the script you used to plot the times? (In a separate PR if you need to clean it up first.)

[lysnikolaou]

Verified everything. My measurements are very similar to those by @pablogsal. Everything that was too difficult to parse before, can be parsed smoothly now. So let's land this.

[gvanrossum]

I confirmed the quadratic behavior as well, and the fix works. Land it.

[pablogsal]

Any chance of also committing the script you used to plot the times? (In a separate PR if you need to clean it up first.)

I will make another PR once I clean it up a bit :)

[pablogsal]

Thanks for the reviews and the confirmation of the problem!