Skip to content

Reenable unboxed values #969

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.

Sign up for GitHub

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

Jump to bottom
Merged
kmod merged 4 commits into from
Oct 15, 2015
Merged

Reenable unboxed values #969

kmod merged 4 commits into from
Oct 15, 2015

Conversation

@kmod
Copy link
Collaborator

@kmod kmod commented Oct 15, 2015

Feels like more work than it was worth:

           django_template3.py             2.2s (2)             2.2s (2)  -0.1%
                 pyxl_bench.py             2.0s (2)             1.9s (2)  -3.6%
     sqlalchemy_imperative2.py             2.4s (2)             2.4s (2)  +0.0%
       django_template3_10x.py            14.3s (2)            14.4s (2)  +0.9%
             pyxl_bench_10x.py            15.4s (2)            15.1s (2)  -2.5%
 sqlalchemy_imperative2_10x.py            18.5s (2)            18.8s (2)  +1.3%
                       geomean                 6.0s                 5.9s  -0.7%

For now unboxed values can't get passed between BBs (compiler types don't have a hook to specify how to phi themselves, but that shouldn't be too hard to add if we need it). Even with this limitation we seem to get most (but not all) of the benefit.

I think there's still some cases that this is missing, but after working on this for a few days I think it's time to check in what's working and I'll just fix any issues if we notice any.

kmod added 4 commits October 15, 2015 07:21
@kmod
Another unboxed-vals test
0ce6a6b 
This will fail even with the current workaround
@kmod
Reenable unboxed ints
affb88b 
- Change INT to a type that remembers whether it was boxed or not.
- We still need a way of specifying a plain unboxed int, so add a
"phony" UNBOXED_INT.  UNBOXED_INT is not usable as a variable type,
but can be used to specify argument and return types.  It's up to
the receiver (irgen or type analysis) to convert UNBOXED_INT to
a usable INT.  Right now that's only guarded with a number of
asserts (and not through, say, the C++ type system).
@kmod
Do the same for float
a303f67
@kmod
ellipsis fix
96555a1
@smarx
Copy link

smarx commented Oct 15, 2015

Automated message from Dropbox CLA bot

@kmod, thanks for the pull request! It looks like you haven't yet signed the Dropbox CLA. Please sign it here and update the thread so we can consider merging your code.

@Daetalus
Copy link
Contributor

Daetalus commented Oct 15, 2015

The message from Dropbox CLA bot.... LOL. This function is just enabled?

@undingen
Copy link
Contributor

undingen commented Oct 15, 2015

from what I understood this PR looks good to me :-)

kmod added a commit that referenced this pull request Oct 15, 2015
@kmod
Merge pull request #969 from kmod/unboxed3
464c98e 
Reenable unboxed values
@kmod kmod merged commit 464c98e into pyston:master Oct 15, 2015
@kmod
Copy link
Collaborator Author

kmod commented Oct 15, 2015

well the bot was right, I hadn't signed the CLA :P

@kmod
Copy link
Collaborator Author

kmod commented Oct 15, 2015

@undingen thanks for looking at the pr :)

Daetalus
Daetalus reviewed Oct 16, 2015
View reviewed changes
src/analysis/type_analysis.cpp
Copy link
Contributor

@Daetalus Daetalus Oct 16, 2015

Choose a reason for hiding this comment

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

I don't quite understand these code for now. But if my understanding was correct. This change cause the error in #971. (I am reading the JIT part source code in these days. Hope I could gather enough questions before 0.4 release. )

Copy link
Collaborator Author

@kmod kmod Oct 16, 2015

Choose a reason for hiding this comment

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

Hmm yeah this PR did introduce that. Can you try merging #973 to see if that helps?

Copy link
Contributor

@Daetalus Daetalus Oct 16, 2015

Choose a reason for hiding this comment

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

Thanks! I will try that.

@kmod kmod deleted the unboxed3 branch November 3, 2015 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kmod @smarx @Daetalus @undingen