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

Preparse old-style octals as strings #17808

Closed
jdemeyer opened this issue Feb 19, 2015 · 27 comments
Closed

Preparse old-style octals as strings #17808

jdemeyer opened this issue Feb 19, 2015 · 27 comments

Comments

@jdemeyer
Copy link

To solve #17807, we preparse 0100 as Integer('0100') instead of Integer(0100): thanks to #17413, this will give a deprecation warning so that users should know something funny is going on:

sage: 0100
/usr/local/src/sage-git/local/lib/python2.7/site-packages/IPython/core/interactiveshell.py:2883: DeprecationWarning: use 0o as octal prefix instead of 0
If you do not want this number to be interpreted as octal, remove the leading zeros.
See http://trac.sagemath.org/17413 for details.
  exec(code_obj, self.user_global_ns, self.user_ns)
64

We also preparse new-style binary/octal/hex strings as integers instead of strings, which should be faster, see [comment:5].

Component: python3

Author: Jeroen Demeyer

Branch/Commit: 52fd0ce

Reviewer: Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/17808

@jdemeyer jdemeyer added this to the sage-6.6 milestone Feb 19, 2015
@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

Branch: u/jdemeyer/ticket/17808

@jdemeyer
Copy link
Author

Commit: 533af4c

@jdemeyer
Copy link
Author

Author: Jeroen Demeyer

@jdemeyer
Copy link
Author

New commits:

533af4cPreparse integers as strings

@nbruin
Copy link
Contributor

nbruin commented Feb 22, 2015

comment:5

I would think the slow-down for small integers makes the proposed changes very unattractive. The whole idea of preparsing 100 as Integer(100) is that in the AST and the generated byte code, the constant 100 is already stored as a numerical one, and hence conversion is much faster.

For instance, in code like:

sage: %timeit L=[Integer('1000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000') for i in range(10000r)]
100 loops, best of 3: 11.4 ms per loop
sage: %timeit L=[Integer(1000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000) for i in range(10000r)]
100 loops, best of 3: 4.66 ms per loop

you really start to notice this.

preparse_file mitigates issues like this a little bit by factoring out numeric constants, so that their conversion doesn't happen in inner loops.

If you want to store your numerical constants as strings in the byte code and have fast conversion, you should probably store them in hex. It's a little more work in the preparser, but the result is more compact and conversion to a numerical constant is truly linear in number of bits.

@jdemeyer
Copy link
Author

comment:6

Replying to @nbruin:

The whole idea of preparsing 100 as Integer(100) is that in the AST and the generated byte code, the constant 100 is already stored as a numerical one

Is that really important? The "timeit" example is of course a bit artificial.

@nbruin
Copy link
Contributor

nbruin commented Feb 22, 2015

comment:7

Replying to @jdemeyer:

Replying to @nbruin:

The whole idea of preparsing 100 as Integer(100) is that in the AST and the generated byte code, the constant 100 is already stored as a numerical one

Is that really important? The "timeit" example is of course a bit artificial.

I think so. Storing the numerical constant as a string forces the decimal-to-binary conversion to happen at runtime. If you just write it as a python integer, the conversion happens at parsing, so by the time it's an ast and the compiler looks at it, you're already dealing with an integer.

Unfortunately, python doesn't permit compile-time macros (perhaps if we're ever going to rewrite our preparser to properly parse, we can combine it with MacroPy and then we can have Integer object creation at compile time)

I am fairly certain that nearly all code that gets run with sage has TONS of small integer literals in it. Probably for many applications, people would get better performance writing 100r, but of course nobody will. This ticket would deteriorate the much more common situation in favour of a small gain in the extremely rare situation that someone wants to include an incredibly large integer literal in their code (talk about artificial--I don't think having a (small) numerical literal in an inner loop is artificial at all!). And the extremely rare situation can be solved by writing quotes already (and by the time you're writing such a long string you might as well convert it to hex anyway, in which case Python and GMP are (at least asymptotically) comparable.

If you use preparse_file instead of preparse, you'll see that numerical constants get pushed outside (which has its own set of problems, see #11542), which mitigates the string-conversion-in-inner-loop.

@jdemeyer
Copy link
Author

comment:8

Fine, you convinced me.

@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title Preparse integers as strings Preparse old-style octals as strings Feb 24, 2015
@jdemeyer

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 24, 2015

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f72c449Preparse old-style octal numbers as strings

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 24, 2015

Changed commit from 533af4c to f72c449

@fchapoton
Copy link
Contributor

Changed commit from f72c449 to b469c4c

@fchapoton
Copy link
Contributor

Changed branch from u/jdemeyer/ticket/17808 to public/ticket/17808

@fchapoton
Copy link
Contributor

New commits:

293c474Merge branch 'u/jdemeyer/ticket/17808' into 6.7.b1
b469c4ctrac #17808 taking care of pt tutorial

@fchapoton fchapoton modified the milestones: sage-6.6, sage-6.7 Apr 19, 2015
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 2, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

b243aa3Merge branch 'public/ticket/17808' into 7.2.b6
52fd0cetrac #17808 handling es and ja tutorial

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 2, 2016

Changed commit from b469c4c to 52fd0ce

@fchapoton fchapoton modified the milestones: sage-6.7, sage-7.2 May 2, 2016
@fchapoton
Copy link
Contributor

comment:17

is the title still acurately describing the content of the ticket ?

@fchapoton fchapoton modified the milestones: sage-7.2, sage-7.3 May 29, 2016
@jdemeyer
Copy link
Author

comment:18

Replying to @fchapoton:

is the title still acurately describing the content of the ticket ?

Yes.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

comment:19

Although the ticket does change a little bit more than that, I added a sentence to the description.

@jdemeyer

This comment has been minimized.

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:21

ok, looks good to me

@vbraun
Copy link
Member

vbraun commented May 31, 2016

Changed branch from public/ticket/17808 to 52fd0ce

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

No branches or pull requests

4 participants