These are chat archives for dropbox/pyston

20th
Jul 2015
Sun
@Daetalus
Jul 20 2015 10:51
Hi @kmod , sorry to bother you. I fixed the all the problems that you mentioned in #719 , except the print(-0.0), I open a issue #724 .
Marius Wachtler
@undingen
Jul 20 2015 17:47
LLVM weekly, phoronix and reddit has something about our last blog entry :-)
Rudi Chen
@rudi-c
Jul 20 2015 18:12
Ok here's an annoying edge case. Two objects, A and B, are in the pending finalization list. The finalizer of A gets called and ends with a call to PyObject_Del, freeing itself. The finalizer of B gets called and triggers a garbage collection. The garbage collector looks at A and crashes because it's no longer in the heap.
I can think of two solutions here. 1) Make PyObject_Del a no-op. I'm not sure what would be the implication of this, but shouldn't all PyObjects be in the heap => they will all get GC'ed without explicit freeing?
2) If (1) is not viable, check if the object has been freed after every finalizer call. This is hacky but C extension code can do anything theoretically.
Travis Hance
@tjhance
Jul 20 2015 18:23
link to reddit post? couldn’t find it
but hasn't any comments
Rudi Chen
@rudi-c
Jul 20 2015 18:43
Another issue: what do we want the semantics of the del keyword to be? 1) Remove all references to the object and let the GC free it eventually. 2) Free the object immediately.
I'm not sure how hard it would be to do (1) properly, because we need to make sure that all references are removed (there might be stuff remaining in inline caches or whatnot?).
Travis Hance
@tjhance
Jul 20 2015 18:44
I think del is not supposed to do anything like free the object, it just removes the reference to the object
Rudi Chen
@rudi-c
Jul 20 2015 18:44
As for (2) this means we have another way of calling finalizers.
Travis Hance
@tjhance
Jul 20 2015 18:44
e.g. del a just makes the local variable a become undefined
del d[key] removes key from the dict
Rudi Chen
@rudi-c
Jul 20 2015 18:45
 class C(object):      
     def __del__(self):
         print "banana"

 c = C()               
 d = c                 
 del c
In CPython, this will print "banana"
Wait actually
Travis Hance
@tjhance
Jul 20 2015 18:45
that’s because it happens to be removing the last reference [EDIT: wrong - see below]
Chris Toshok
@toshok
Jul 20 2015 18:45
It shouldn't- d should have reffed it
Travis Hance
@tjhance
Jul 20 2015 18:45
oh wait
Rudi Chen
@rudi-c
Jul 20 2015 18:45
class C(object):      
    def __del__(self):
        print "banana"

c = C()               
d = c                 
print "Before"        
del c                 
print "After"
Travis Hance
@tjhance
Jul 20 2015 18:45
oh yeah but it will remove the d reference later when the program exits
Rudi Chen
@rudi-c
Jul 20 2015 18:46
"Before" "After" "banana" ok we're good you're right
Chris Toshok
@toshok
Jul 20 2015 18:48
The gc looks at a because
Err oops. The gc looks at a again because it's still in the list?
Rudi Chen
@rudi-c
Jul 20 2015 18:49
Yeah, I only remove objects from the pending_finalization_list after all the finalizers have been called.
This is not too hard to fix, but it's only a partial fix.
I'm not sure if we should allow explicit freeing at all.
Chris Toshok
@toshok
Jul 20 2015 18:50
We shouldn't imo
The gc will clear up the conservatively allocated blocks just as well as python/conservative_python blocks
Kevin Modzelewski
@kmod
Jul 20 2015 18:51
oh nice the object caching post is up to 3.5k views :)
ok feel free to start writing your status updates:
unfortunately I can't make that world-readable :/
Rudi Chen
@rudi-c
Jul 20 2015 18:56
If you put it in your personal notes account you can make it world-readable.
Not sure if that's important though.
Chris Toshok
@toshok
Jul 20 2015 18:57
Appt ran late - I'm still in car ~5 minutes away. Scared to edit notes docs on this phone :)
Kevin Modzelewski
@kmod
Jul 20 2015 18:58
oh hmm looks like I would have to sign out and then back in? maybe we can try that next week :/
Rudi Chen
@rudi-c
Jul 20 2015 18:58
Notes doesn't support 2 account yet ;-;
I usually open up a Google Incognito window so I can have two Notes account open at once.
Kevin Modzelewski
@kmod
Jul 20 2015 19:17
@toshok yeah agreed on the flaky test thing
I'll keep looking into it as I have time but if anyone else wants to pitch in that will be appreciated :)
and yeah I don't think LTO helps that much on its own (at least it didn't for CPython), but I wonder if it becomes much more helpful once PGO is enabled (ie it can make some smart cross-TU inlining decisions)
Chris Toshok
@toshok
Jul 20 2015 19:18
yeah, exactly
Kevin Modzelewski
@kmod
Jul 20 2015 19:22
re tests, I wonder if we have some underlying bugs that don't hit that often
Travis Hance
@tjhance
Jul 20 2015 19:23
well, there’s this test called threading_local.py
Kevin Modzelewski
@kmod
Jul 20 2015 19:25
heh yeah :)
I think getting some more integration tests would be nice as well
it's nice that they catch so many issues, but they're not designed to be comprehensive so it's hard to know what percentage of bugs they catch
and a simple improvement is to add more of them :)
Marius Wachtler
@undingen
Jul 20 2015 19:25
there is sadly almost definitely something wrong with the ASTInterpreter GC, for example undingen/pyston@214022d without the RootedBox some tests will fail. But this var should live in regs/stack I don't know why they need explicit stackroots
and I encountered several similar problems in this area
To me it would make more sense if we loose refs to the values inside the >3 args array. But we loose it to the normal args which get passed in regs (and AFAIK only in Debug build!)
because it happens only in debug build it's can't be some fancy compiler optimization...
Travis Hance
@tjhance
Jul 20 2015 19:30
that array is always either in the stack or in an object though, and there’s always a pointer to it, so theoretically it shouldn’t be a problem
Kevin Modzelewski
@kmod
Jul 20 2015 19:30
oh hmm I would have guessed the opposite, that it's easier to lose refs to the register arguments
Chris Toshok
@toshok
Jul 20 2015 19:30
is there another typecasted function lying around that I missed?
yeah it’s quite conceivable the RA (even in a debug build) will say “you don’t need this value you’re passing in $rdi anymore, so I won’t copy it to the stack before calling the function"
Marius Wachtler
@undingen
Jul 20 2015 19:33
yeah normally I would also think the registers are more temporary but this functions stores one arg after another inside the sym table / globals etc... (And it's not because I changed it to not use any GC mem because with the patch which got reverted last week we encountered nearly the same bug)
Chris Toshok
@toshok
Jul 20 2015 19:34
(pgo instrumentation builds are so slow :( )
Kevin Modzelewski
@kmod
Jul 20 2015 19:56
hi @Daetalus sorry for missing your message, yeah it looks like the -0.0 issue is probably unrelated to your pr, feel free to skip that one
I'll be happy to review the rest of the changes :)
Marius Wachtler
@undingen
Jul 20 2015 20:25
github PR status site has slightly changed the layout
Chris Toshok
@toshok
Jul 20 2015 20:36
oh the PR status you mean?
err, the inline status at the bottom?
i like it - it was never very clear whether something could be merged. and the green “everything is ok!” was contradictory
Marius Wachtler
@undingen
Jul 20 2015 20:39
yes the little status at the bottom which displays branch is up-to-date and switches between yellow and red and never becomes green :-P
Chris Toshok
@toshok
Jul 20 2015 20:45
i wish build systems relied less on string replacement/concatenation :/
most interesting because: box is php?!
those graphs are pretty amazing
Rudi Chen
@rudi-c
Jul 20 2015 21:06
Getting rid of explicit freeing calls not only works without issues but doesn't cause a performance hit, nice.
Not sure if there's a memory leak anywhere though. Shouldn't be in theory.
Chris Toshok
@toshok
Jul 20 2015 22:17
whoah
clang-pgo did sqlalchemy_imperative.py in 1.3s
(without crashing)
Travis Hance
@tjhance
Jul 20 2015 22:18
what was it before?
Chris Toshok
@toshok
Jul 20 2015 22:18
3.9s for clang, 3.6s for gcc-pgo
Travis Hance
@tjhance
Jul 20 2015 22:18
you’re not thinking of sqlalchemy_imperative2.py?
Chris Toshok
@toshok
Jul 20 2015 22:19
ahh, phew. thanks. i had run the original runs against minibenchmarks/sqlalchemy_imperative.py, which was renamed when copying over to pyston-perf
thanks for ruining that -66%, dude
Travis Hance
@tjhance
Jul 20 2015 22:20
sorry:(
Marius Wachtler
@undingen
Jul 20 2015 22:38
would not complain about a -66% :-D
Chris Toshok
@toshok
Jul 20 2015 23:16
compilation terminated.
The bug is not reproducible, so it is likely a hardware or OS problem.
Travis Hance
@tjhance
Jul 20 2015 23:16
how does it know that it’s not reproducible?
Kevin Modzelewski
@kmod
Jul 20 2015 23:24
so as I look more into the django_template benchmark, I'm starting to think it's not quite what we want to test :/
I didn't realize this, but it's only repeatedly parsing the template, and parsing does a whole bunch of weird stuff
like I guess creating custom classes and what not
I created a new django_template2.py benchmark in pyston-perf which tests the template rendering
not sure what to make of it yet, but we are 176% slower than cpython (pypy is 126% slower)
hopefully there is some low-hanging fruit from code paths we haven't looked at yet :)
Chris Toshok
@toshok
Jul 20 2015 23:29
at the time, parsing is what dominated a given request. guessing we’ve made improvements where that’s no longer the case?
either way, more/different benchmarks :+1:
Kevin Modzelewski
@kmod
Jul 20 2015 23:29
does django have to parse the template for each request?
Chris Toshok
@toshok
Jul 20 2015 23:30
i don’t remember :/ i do remember fixing the LRU caching layer at some point
so there’s some caching, but unsure what of
Chris Toshok
@toshok
Jul 20 2015 23:58
gcc-pgo build is worse than normal release build on that benchmark
wonder why