These are chat archives for dropbox/pyston

14th
Jul 2015
Kevin Modzelewski
@kmod
Jul 14 2015 00:10 UTC
yeah, Python has a bunch of rules that depend on whether or not a class was heap-allocated
and we mostly ignore that logic
usually if CPython wants to use tp_as_sequence, they statically-allocate a separate PySequencMethods struct and then put that pointer into the PyTypeObject struct
Travis Hance
@tjhance
Jul 14 2015 00:11 UTC
right
Kevin Modzelewski
@kmod
Jul 14 2015 00:11 UTC
but I feel like it should be ok to make them static BoxedClass's (non-Heap)
but yeah we can't depend on tp_as_sequence getting initialized by the BoxedHeapClass constructor
Travis Hance
@tjhance
Jul 14 2015 00:12 UTC
so what’s the right way to initialize it, then?
Chris Toshok
@toshok
Jul 14 2015 00:12 UTC
<jasone> Holy crap that compiled fast, toshok
<jasone> I was prepared to wait over an hour.
:clap: @dagar
Kevin Modzelewski
@kmod
Jul 14 2015 00:13 UTC
err well I'm not sure what the rest of it looks like
but there shouldn't be any reason it couldn't just work the same way say, the unicode type does
Chris Toshok
@toshok
Jul 14 2015 00:14 UTC
won’t we then need to visit them conservatively?
Kevin Modzelewski
@kmod
Jul 14 2015 00:15 UTC
well we probably shouldn't call PyType_Ready on them
but just in terms of getting the data structures allocated and initialized, that could happen the same way
oh but I guess we would want to call our constructor on it
Chris Toshok
@toshok
Jul 14 2015 00:44 UTC
well, I can fix the perf regression caused by the 1 type registered by the grp module
by removing the non heap root machinery entirely :/
Travis Hance
@tjhance
Jul 14 2015 00:57 UTC
hm if the non heap root stuff is slow, that doesn’t bode well for the statically allocated type objects :(
Chris Toshok
@toshok
Jul 14 2015 00:57 UTC
might be able to do something about it by having another layer - min/max/unordered_set per .so (and one for the executable)
there’s very little that actually relies on the nonheap-root status (other than for assertions)
Travis Hance
@tjhance
Jul 14 2015 00:58 UTC
oh wait so what’s the issue?
Chris Toshok
@toshok
Jul 14 2015 00:58 UTC
we have ~150 non-heap roots for django template
they’re in the range 0x1600000-0x17f0000 or something
every time we GCVisitor::visit a pointer we check if it’s in that range (we keep min/max), and if it is, we look in the unordered_set to make sure it was registered
the grp .so registers a nonheap root that’s at like 0x2a000000000
or something really high
actually inside gdb it gets mapped up at 0x7fffff….. which means we look up every pointer in the unordered_set before then looking in our actual heap for it
Travis Hance
@tjhance
Jul 14 2015 01:02 UTC
I didn’t realize gdb affected where stuff got mapped
Kevin Modzelewski
@kmod
Jul 14 2015 01:04 UTC
could we just make these checks into asserts?
Chris Toshok
@toshok
Jul 14 2015 02:27 UTC
yeah, going to reorder things such that we check our heap bounds first
then we can assert
trouble is, that 0x2a…. address, though. that could have been in the large heap
which is a problem :/
think I’m going to make the arenas just reserve their entire mappings at startup
Travis Hance
@tjhance
Jul 14 2015 02:31 UTC
how do I re-throw an exception? I tried a bare throw and I got __cxa_rethrow() unimplemented; please don't use barethrow' in Pyston!`
should I just do throw e where e is the exception object I caught?
Chris Toshok
@toshok
Jul 14 2015 02:32 UTC
yeah
Travis Hance
@tjhance
Jul 14 2015 02:32 UTC
k cool
Travis Hance
@tjhance
Jul 14 2015 03:24 UTC
do tpp_* functions throw exceptions?
Kevin Modzelewski
@kmod
Jul 14 2015 04:07 UTC
yeah
webus
@webus
Jul 14 2015 06:51 UTC
how is current progress in pyston ? can i use it now in production ?
Marius Wachtler
@undingen
Jul 14 2015 08:35 UTC
Pyston progresses very rapid. we are now able to run large applications with minor problems. And getting new large libs to run if they don't work out of the box it's mostly a one day job. But main focus is currently on performance again. But as of use in production: I wouldn't recommend using it yet in production. It coming closer but I'm pretty sure if you run it longer you will encounter issues.
Sun
@Daetalus
Jul 14 2015 08:37 UTC
How many people work for Pyston as full time job?
Marius Wachtler
@undingen
Jul 14 2015 08:56 UTC
~5 people
Sun
@Daetalus
Jul 14 2015 08:56 UTC
Thanks!
Marius Wachtler
@undingen
Jul 14 2015 09:31 UTC
@kmod what tests are failing / how can I reproduce the issue?
Kevin Modzelewski
@kmod
Jul 14 2015 09:47 UTC
doh it looks like they've been failing for a while
I don't know why I didn't get any emails
at least on master, I've been getting failures with make run_release_gcc_babel_test
(you need to pull the thing I just pushed to master that adds release_gcc)
Sun
@Daetalus
Jul 14 2015 09:48 UTC
@kmod , please take a bit time to look at #683, because I almost rewrite pow functions in int and long object. The test rely on convert long to float.
Another question, in test/cpython/test_pow, def test_bug643260, due to old-style class not support __rpow__ yet, I change the class to new style class. Is that OK?
I already implement these 'xxx' stuff in my local repos. But I need to improve it along with oldstyle `_pow` support.
Kevin Modzelewski
@kmod
Jul 14 2015 09:51 UTC
ok I will take a look :)
Sun
@Daetalus
Jul 14 2015 09:52 UTC
I already implement '__xxx__` in old style class, sorry for the typo.
Kevin Modzelewski
@kmod
Jul 14 2015 09:57 UTC
ok it seems to be the interpreter gc change
that's what Travis-CI says was the first red build
and if I revert it, the babel test passes again
Kevin Modzelewski
@kmod
Jul 14 2015 10:25 UTC
I think we might have to pass the wrapper around
instead of passing the interpreter pointer around
if we only have the interpreter pointer, then it won't trace through to see the wrapper and keep the interpreter alive
just a theory, the babel failure is pretty obscure
Chris Toshok
@toshok
Jul 14 2015 18:18 UTC
man, this grp.so thing is making no sense at all
unchanged, the addition of grp.so causes GCVisitor::visit time to spike in perf output (to 1450, 8.9% of samples) due to every single gc heap pointer also being in the range of min-max_nonheap_root
oh also note before that change, SmallArena::allocationFrom is at 126/0.78%
after that change, GCVisitor::visit drops to 298,1.88%, but SmallArena::allocationFrom jumps to 898/5.70%
Chris Toshok
@toshok
Jul 14 2015 18:23 UTC
and overall there’s barely a reduction in runtime
with that change I would expect GCVisitor::visit to take slightly more time than it did before (it has to do 7 comparisons - 2 for each arena, then one for the null check - instead of just 2 for the min-max non heap roots)
but i wouldn’t expect any additional time spent in SmallArena::allocationFrom
Sun
@Daetalus
Jul 14 2015 18:27 UTC
The current work of Pyston is focus on performance?
Chris Toshok
@toshok
Jul 14 2015 18:27 UTC
even removing grp.so (and the others ones I added for this PR) doesn’t help the situation
@Daetalus yes
Marius Wachtler
@undingen
Jul 14 2015 18:28 UTC
because small_arena.contains() should return false for this cases. so there shouldn't be any additionals allocationFrom calls??
Chris Toshok
@toshok
Jul 14 2015 18:28 UTC
correct
Sun
@Daetalus
Jul 14 2015 18:29 UTC
Thanks, I want to make some contributions, start from let more tests could pass. And already in progress.
Marius Wachtler
@undingen
Jul 14 2015 18:31 UTC
Getting more tests to pass is always very welcome :-)
Sun
@Daetalus
Jul 14 2015 18:32 UTC
But the PR feedback/review seems slow...
Marius Wachtler
@undingen
Jul 14 2015 18:36 UTC
btw: guys sorry for breaking the build. didn't realize the commit was causing the problem
Chris Toshok
@toshok
Jul 14 2015 18:54 UTC
more fun: changing Arena::contains to return true for anything inside of the constant range of [start, start+size) instead of doing a load of cur (the current high watermark) + comparison, then doing the cur compare inside of each ::allocationFrom drops it from 5.70% to 5%, but ups the cycles to 946.
the div instruction in SmallArena::allocationFrom (from int obj_idx = offset / size;) shows as taking 50% of the time we spend in that method
Kevin Modzelewski
@kmod
Jul 14 2015 19:21 UTC
Sorry @Daetalus! that's my bad.
also, not sure if this was happening here, but one thing that surprised me about github (but I guess it makes sense) is that it doesn't send out notifications when you update the PR
Rudi Chen
@rudi-c
Jul 14 2015 19:22 UTC
Oh interesting. So whenever we update a PR, we should always comment?
Kevin Modzelewski
@kmod
Jul 14 2015 19:23 UTC
if it's ready for review again
also, marius's blog post is now live!
Chris Toshok
@toshok
Jul 14 2015 19:24 UTC
yeah, lack of notifications is a pain :/ it does show as synchronized in the gitter activity feed (which scrolls by too quick), and the PR list can be sorted by most/least recently updated, but neither of those are a substitute
Marius Wachtler
@undingen
Jul 14 2015 19:25 UTC
:-)
Kevin Modzelewski
@kmod
Jul 14 2015 19:25 UTC
I think one thing that makes it tricky is that not all updates signify that the PR is ready for review again
ex if the build fails
Kevin Modzelewski
@kmod
Jul 14 2015 19:33 UTC
I was thinking of introducing a labeling system so that it could be more clear what state things are in (or maybe more importantly, what state other people thing things are in)
ex things start off as "pending-review", then after review go back to "pending-updates", etc
I thought it would be too heavyweight but maybe it's worth it?
Rudi Chen
@rudi-c
Jul 14 2015 19:34 UTC
I think it's a good idea to try. It would help with the communication issue (the "am I supposed to be doing something right now?" for both parties).
Sun
@Daetalus
Jul 14 2015 20:17 UTC
@kmod #683 updated and pass the Travis Cl test.
Kevin Modzelewski
@kmod
Jul 14 2015 20:20 UTC
merged :)
ok I submitted the blog post to HN
Sun
@Daetalus
Jul 14 2015 20:22 UTC
Thanks @kmod ! Another question, I am working on pow issue in test/cpython/test_pow, function def test_bug643260(self):, due to the old-style class lack of \_\_rpow\_\_ support. It could not pass the test. Could I change it to new style class in order to let the pow test pass?
Kevin Modzelewski
@kmod
Jul 14 2015 20:24 UTC
oh hmm
I am not a huge fan of that :/
could we do the pow changes for now without re-enabling the test_pow test
and then add old-style class rpow?
Sun
@Daetalus
Jul 14 2015 20:27 UTC
Good idea.
I am already add old-style class rpow(along with other operators) in my fork. But it seems need some improvements.
I will create a PR tomorrow with the pow changes first.
Kevin Modzelewski
@kmod
Jul 14 2015 20:32 UTC
awesome :)
yeah it would be easier to just change the test, but I think we have to be quite against that so that we can later advertise that we pass the cpython test suite
Sun
@Daetalus
Jul 14 2015 20:33 UTC
According to the release interval, 0.4 comming soon?
Kevin Modzelewski
@kmod
Jul 14 2015 20:34 UTC
hopefully! :)
I'd like us to hit some performance targets, but that's taking longer than we expected
Sun
@Daetalus
Jul 14 2015 20:35 UTC
Hope I have enough time to let more tests pass before 0.4 release.
Kevin Modzelewski
@kmod
Jul 14 2015 20:37 UTC
Also, I went through the PRs and tagged the ones that I think I'm reviewing along with what I think the status is
though it looks like people without write-access to the repo don't have the ability to change the labels
or assign a reviewer
so we'll see how well this works out
Chris Toshok
@toshok
Jul 14 2015 20:44 UTC
so the workflow for submissions is something like: after a PR passes travis-ci and it’s ready for review, the submitter either adds the label (if they have write access), or requests review?
Kevin Modzelewski
@kmod
Jul 14 2015 20:47 UTC
yeah
I imagine it'll mostly be as-needed
if the pr passes ci, I'll usually review it right away
err, if I get to it :P
Kevin Modzelewski
@kmod
Jul 14 2015 20:54 UTC
looking at the blog-post stats showed me this interesting site:
Chris Toshok
@toshok
Jul 14 2015 20:55 UTC
scared to click
oh wow, much better
Marius Wachtler
@undingen
Jul 14 2015 21:27 UTC
mmh I'm getting now a lot of asserts:
if ((uintptr_t)p < SMALL_ARENA_START || (uintptr_t)p >= HUGE_ARENA_START + ARENA_SIZE) {
        ASSERT(isNonheapRoot(p), "%p", p);
        return;
    }
problem is that we called visit with 0 pointers. and they fail the isNonheapRoot check
Chris Toshok
@toshok
Jul 14 2015 21:29 UTC
yeah
dropbox/pyston#701
Marius Wachtler
@undingen
Jul 14 2015 21:34 UTC
:-)
Chris Toshok
@toshok
Jul 14 2015 21:34 UTC
sorry about that :/
Marius Wachtler
@undingen
Jul 14 2015 21:38 UTC
this babel ASTInterpreterGCWrapper bug is really annoying.
Chris Toshok
@toshok
Jul 14 2015 21:53 UTC
hm, so the pure python impl of mercurial works, but compiling the native modules fails due to our use of bool in cpython headers
well, “works”, i haven’t really done much testing
Marius Wachtler
@undingen
Jul 14 2015 21:58 UTC
so we are getting a mercurial benchmark? :-P
Chris Toshok
@toshok
Jul 14 2015 21:58 UTC
i was going to look into it :)
Chris Toshok
@toshok
Jul 14 2015 22:09 UTC
this might take too long… mercurial-3.4.1-install/bin/hg clone https://hg.mozilla.org/mozilla-central/ firefox
Marius Wachtler
@undingen
Jul 14 2015 22:11 UTC
  • we should build it just to make sure the checkout was successful :-D
Chris Toshok
@toshok
Jul 14 2015 22:14 UTC
haha. that would likely swallow up any perf difference between cpython and pyston too
Kevin Modzelewski
@kmod
Jul 14 2015 22:29 UTC
we got >1k views on the blog post :)
Travis Hance
@tjhance
Jul 14 2015 22:29 UTC
oh, one of those was me
Chris Toshok
@toshok
Jul 14 2015 22:29 UTC
how long has it been up?
just a few hours?
Kevin Modzelewski
@kmod
Jul 14 2015 22:29 UTC
yeah
Travis Hance
@tjhance
Jul 14 2015 22:30 UTC
it’s on the front page of slacker news right now
Chris Toshok
@toshok
Jul 14 2015 22:30 UTC
cringe so mercurial has RES 8.743g so far
Travis Hance
@tjhance
Jul 14 2015 22:30 UTC
what does that mean?
Chris Toshok
@toshok
Jul 14 2015 22:31 UTC
i don’t know how much memory mercurial normally uses when doing a clone (with cpython), but that’s the resident set for pyston running mercurial
Travis Hance
@tjhance
Jul 14 2015 22:31 UTC
oh that’s the memory usage
Chris Toshok
@toshok
Jul 14 2015 22:31 UTC
yeah
first comment on hackernews: python3
Kevin Modzelewski
@kmod
Jul 14 2015 22:35 UTC
we clearly need "when is python3 happening" pinned to our front page :P
Chris Toshok
@toshok
Jul 14 2015 22:37 UTC
damn:
transaction abort!
rollback completed
abort: connection ended unexpectedly
clearly I need a small repo to clone :)
Travis Hance
@tjhance
Jul 14 2015 22:51 UTC
python3: the intern project
Marius Wachtler
@undingen
Jul 14 2015 22:53 UTC
concerning the ASTInterpreterGCWrapper I still don't understand why I have to pass the wrapper around every where (but the issue is fixed when I pass it to the baseline jit helper methods etc..)
Because I always allocate the ASTInterpreter instance on the stack. this means the ASTInerpreter::wrapper field should also be on the stack and should get scanned therefore. I don't pass the wrapper around further down but I thought that's not necessary because a few caller stack frames higher up there will always be the ASTInterpreter instance including the wrapper laying around on the stack...
Kevin Modzelewski
@kmod
Jul 14 2015 22:56 UTC
oh hrm yeah
maybe I was off with that theory