[issue11462] Peephole creates duplicate and unused constants

Terry J. Reedy added the comment: Eugene has started work on AST optimizer in #11549 -- nosy: +terry.reedy superseder: -> Rewrite peephole to work on AST ___ Python tracker ___

[issue11462] Peephole creates duplicate and unused constants

Eugene Toder added the comment: Raymond, you can be assured that I'm not developing this patch, unless I'm told it has chances to be accepted. I don't like to waste my time. On a related note, your review of my other patches is appreciated. -- ___

[issue11462] Peephole creates duplicate and unused constants

Raymond Hettinger added the comment: Please cease development of this patch. It has been rejected. The peephole optimizer should be considered somewhat off-limits at this point and any further development efforts directed at an AST optimizer. -- resolution: -> rejected

[issue11462] Peephole creates duplicate and unused constants

Eugene Toder added the comment: Alexander, my patch does 2 optimizations: doesn't insert a new constant if one already exist and removes unused constants after peephole is done. You patch seems to do only the latter. It's very similar, from a quick look at your patch: - My patch doesn't introd

[issue11462] Peephole creates duplicate and unused constants

Alexander Belopolsky added the comment: Eugene, how does your optimization differ from the one proposed in issue2493? -- nosy: +belopolsky ___ Python tracker ___ ___

[issue11462] Peephole creates duplicate and unused constants

Eugene Toder added the comment: Thomas, can you clarify, does loading interns all constants in co_consts, or do you mean that they are mostly small numbers and the like? Without interning I think that in-memory size difference is even bigger than on-disk, as you get one entry in tuple and the

[issue11462] Peephole creates duplicate and unused constants

Raymond Hettinger added the comment: Sorry Eugene. As the person who designed, implemented, and maintains the peephole optimizer, I need to reject the patch. If better constant folding is wanted, the work needs to be done with AST, not with bytecode. I would welcome a patch. Thomas, thanks

[issue11462] Peephole creates duplicate and unused constants

Thomas Wouters added the comment: What is the actual performance impact of this change? As far as I can see the difference is almost entirely in .pyc (on-disk) size, as when loading most of these constants are interned anyway. Even the on-disk change is minimal - and I say that as a person wh

[issue11462] Peephole creates duplicate and unused constants

Changes by Thomas Wouters : Removed file: http://bugs.python.org/file21086/nonmethod-warn-nongit.diff ___ Python tracker ___ ___ Python-bugs-l

[issue11462] Peephole creates duplicate and unused constants

Changes by Thomas Wouters : Added file: http://bugs.python.org/file21086/nonmethod-warn-nongit.diff ___ Python tracker ___ ___ Python-bugs-lis

[issue11462] Peephole creates duplicate and unused constants

Changes by Raymond Hettinger : -- nosy: +twouters -rhettinger ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: h

[issue11462] Peephole creates duplicate and unused constants

Eugene Toder added the comment: > Right now, the pattern is tokenize -> parse -> AST -> genbytecode -> > peephole optimization (which disassembles the bytecode, analyzed it > and rewrites it) -> final bytecode. The correct pattern is tokenize > -> parse -> AST -> optimize -> genbytecode -> pee

[issue11462] Peephole creates duplicate and unused constants

Antoine Pitrou added the comment: > > _PyCode_AddObj() should be added to Include/code.h > Should it be declared as PyAPI_FUNC too? This will make it > unnecessarily exported (when patch in Issue11410 is merged), so I > wanted to avoid that. I guess we don't usually try to optimize that, but yo

[issue11462] Peephole creates duplicate and unused constants

Eugene Toder added the comment: > _PyCode_AddObj() should be added to Include/code.h Should it be declared as PyAPI_FUNC too? This will make it unnecessarily exported (when patch in Issue11410 is merged), so I wanted to avoid that. btw, that you for reviewing the patch. -- __

[issue11462] Peephole creates duplicate and unused constants

Eugene Toder added the comment: assert() doesn't quite work here. I need to check that this entry in the table is not used in the next loop. I'd need to put assert in that loop, but by that time I have no easy way to tell which entries are bad, unless I mark them in the first loop. so I mark

[issue11462] Peephole creates duplicate and unused constants

Antoine Pitrou added the comment: > > - why the "#ifndef NDEBUG" path? > These entries in the table should not be used, but if something slips > through and uses one of them, it's much easier to tell if we remap to > invalid value. As this is an internal check, I didn't want it in > release mode

[issue11462] Peephole creates duplicate and unused constants

Eugene Toder added the comment: > - why the "#ifndef NDEBUG" path? These entries in the table should not be used, but if something slips through and uses one of them, it's much easier to tell if we remap to invalid value. As this is an internal check, I didn't want it in release mode. If this

[issue11462] Peephole creates duplicate and unused constants

Antoine Pitrou added the comment: > Also, the peephole optimizer was developed before the AST branch was > added to Python. Constant folding more properly belongs at the AST > level, not in the peepholer itself. Our strategy is to move as much > as possible into an AST optimizer and to stop bu

[issue11462] Peephole creates duplicate and unused constants

Antoine Pitrou added the comment: About dedup_const_planb.patch: _PyCode_AddObj() should be added to Include/code.h, even if it's private (rather than declared extern in peephole.c). About consts_test.patch: "def mapping" deserves a better name, and perhaps a comment or explanation. Same for

[issue11462] Peephole creates duplicate and unused constants

Raymond Hettinger added the comment: I'm disinclined to accept this patch. There is only minor space savings and no actual speed-up. When constant folding was introduced, a design decision was made to ignore the issue of orphan constants. The rationale behind that decision still stands. Th

[issue11462] Peephole creates duplicate and unused constants

Eugene Toder added the comment: Antoine, sure, I'll fix it with any other suggested changes if patches will be accepted. -- ___ Python tracker ___ _

[issue11462] Peephole creates duplicate and unused constants

Antoine Pitrou added the comment: Style nit: you have tabs in your code. Please only use spaces for indentation. On the principle, looks worthwhile, and the changes don't look really complicated. -- nosy: +mark.dickinson ___ Python tracker

[issue11462] Peephole creates duplicate and unused constants

Eugene Toder added the comment: I think the changes are fairly trivial. dedup_const_planb.patch is about 10 lines of new code with all the rest being trivial plubming. unused_consts.patch may look big, but only because I factored out fix ups into a separate function; there are only about 25 l

[issue11462] Peephole creates duplicate and unused constants

Raymond Hettinger added the comment: Sorry Eugene, but I'm not willing to add that much complexity to save a few bytes. This was a known issue and long ago we choose to leave it as-is. In the future when constant folding is done at the AST level, we'll get more extensive optimizations and t

[issue11462] Peephole creates duplicate and unused constants

Eugene Toder added the comment: (test case) -- nosy: +pitrou Added file: http://bugs.python.org/file21077/consts_test.patch ___ Python tracker ___ __

[issue11462] Peephole creates duplicate and unused constants

Changes by Eugene Toder : Added file: http://bugs.python.org/file21076/unused_consts.patch ___ Python tracker ___ ___ Python-bugs-list mailing

[issue11462] Peephole creates duplicate and unused constants

Eugene Toder added the comment: (either plana or planb should be applied) -- Added file: http://bugs.python.org/file21075/dedup_const_planb.patch ___ Python tracker ___

[issue11462] Peephole creates duplicate and unused constants

Changes by Eugene Toder : -- keywords: +patch Added file: http://bugs.python.org/file21074/dedup_const_plana.patch ___ Python tracker ___

[issue11462] Peephole creates duplicate and unused constants

New submission from Eugene Toder : Peephole optimizer performs constant folding, however 1) When it replaces operation with LOAD_CONST it always adds a new constant to co_consts, even if constant with the same value is already there. It also can add the same constant multiple times. 2) It does