These are chat archives for dropbox/pyston

22nd
May 2015
Kevin Modzelewski
@kmod
May 22 2015 00:00
it looks like __del__ = tp_del?
hmm it looks like most Python objects might fall into the category you described?
they'll have a tp_dealloc but no __del__
at least, in cpython
our handling of the destructor slots is definitely incomplete since we've never really looked at it
you'll probably have to spend some time with cpython's typeobject.c :)
Rudi Chen
@rudi-c
May 22 2015 00:05
Yeah, although a lot of their tp_dealloc don't seem to do much. It's also a bit confusing because from the docs and stack overflow answers, I got the impression that tp_dealloc was a destructor and tp_del was for freeing memory (which usually is always the same logic, hence delegated to the parent). In that sense, tp_dealloc corresponds to __del__ more than tp_del but you're right, tp_del == __del__ if you look at the code.
Michael Arntzenius
@rntz
May 22 2015 00:06
@kmod looks like CMake's PATCH_COMMAND actually modifies the source tree directly (i.e. it'll change the files in $HOME/pyston/libunwind); I see no easy way around this
as long as the patch command is idempotent this shouldn't be a problem in practice, but it's kind of ugly since it dirties the source tree
Chris Toshok
@toshok
May 22 2015 00:30
bizarre. a pyston_release built with pyston_deps/gcc-4.8.2 doesn’t ever hit _Unwind_Find_FDE
Rudi Chen
@rudi-c
May 22 2015 00:44
I got tp_del and tp_free mixed up, that's why I was confused -_-
Marius Wachtler
@undingen
May 22 2015 20:25
@tjhance the condition should be && not || together?
Travis Hance
@tjhance
May 22 2015 20:25
well, at most one of attrs_offset and tp_dictoffset will be nonzero
Marius Wachtler
@undingen
May 22 2015 20:28
Oh I think I understood it now: the tp_dictoffset is set when we store the attributes in a dict. And the attrs_offset will be set if we store the attributes directly inside the object? Only one of the two can be set at a given time?
Marius Wachtler
@undingen
May 22 2015 20:49
and tp_basicsize doesn't include the size of the weaklist object* and dict object* if its exists?
Travis Hance
@tjhance
May 22 2015 20:51

the tp_dictoffset is set when we store the attributes in a dict. And the attrs_offset will be set if we store the attributes directly inside the object? Only one of the two can be set at a given time?

Correct.

tp_basicsize doesn't include the size of the weaklist object* and dict object* if its exists

tp_basicsize should include those fields.

Marius Wachtler
@undingen
May 22 2015 20:53
Why does cpython then
    size = base->tp_basicsize;
    if (a->tp_dictoffset == size && b->tp_dictoffset == size)
        size += sizeof(PyObject *);
    if (a->tp_weaklistoffset == size && b->tp_weaklistoffset == size)
        size += sizeof(PyObject *);
?
Travis Hance
@tjhance
May 22 2015 20:54
that’s… a good question
oh I see
it’s because base is the base of a
Marius Wachtler
@undingen
May 22 2015 20:56
oh.. I see
Travis Hance
@tjhance
May 22 2015 20:56
so it’s checking if tp_dictoffset == size (i.e., we have a dict in a but not in base), then increment size
Travis Hance
@tjhance
May 22 2015 21:03
Why does python even have this feature
The ability to change the base seems totally useless
Marius Wachtler
@undingen
May 22 2015 21:08
:-(. At least I now have finally to bother with this stuff. I'm quite afraid to notice how little I know about the object layout.
Travis Hance
@tjhance
May 22 2015 21:10
Yeah I figured it out when I implemented the slots stuff
Kevin Modzelewski
@kmod
May 22 2015 21:10
marius I feel the same way about the virtualenv stuff :P
Marius Wachtler
@undingen
May 22 2015 21:11
:-D
Travis Hance
@tjhance
May 22 2015 21:12
the documentation is pretty informative: https://docs.python.org/2/c-api/typeobj.html
Marius Wachtler
@undingen
May 22 2015 21:16
Now I'm just still not sure how the same_slots_added function has to look like... Looks like the sizewon't be the same as tp_basicsize
Travis Hance
@tjhance
May 22 2015 21:24
why not?
Marius Wachtler
@undingen
May 22 2015 21:27
Looks like I will have to add sizeof(HCAttrs) so size. But before I make myself even more look like a fool I will better test it and check the source :-D
Marius Wachtler
@undingen
May 22 2015 21:35
ok new version: https://gist.github.com/undingen/74f15bbbe2ebbf9fe442 what do you guys think of this one?
Travis Hance
@tjhance
May 22 2015 21:41
via analogy with tp_dictoffset, it looks fine
hmm
it would really suck if somebody tries to set __bases__ and we fail because the layouts are incompatible because one uses tp_dictoffset and the other uses attrs_offset
although again
who actually uses this feature?
Kevin Modzelewski
@kmod
May 22 2015 21:45
I think it's only extension modules
Marius Wachtler
@undingen
May 22 2015 21:45
can a class change from attr_offset to tp_dictoffset? because if the tp_dictoffset can only by set at the base class via capi this should be fine because the base classes have to be the same.
Kevin Modzelewski
@kmod
May 22 2015 21:45
that request having a dict for attributes on their types
err, for tp_dictoffset
not sure who actually tries to change the bases, but I assume that we ran into a case of it which is why we're looking into it...
classes can't change between hcattrs-backed / dict-backed
or, they should be disallowed from it if they can :P
Travis Hance
@tjhance
May 22 2015 21:47

I think it's only extension modules

What is?

Marius Wachtler
@undingen
May 22 2015 21:47
I'm not totally sure what lib was it but I think it was babel or numpy
Travis Hance
@tjhance
May 22 2015 21:48
tp_dictoffset is only for extension modules, but you could imagine someone trying to replace a base from python class -> extension module class
as far as I know
and we would just flat-out not be able to do that
Kevin Modzelewski
@kmod
May 22 2015 22:02
sorry, I meant tp_dictoffset is only for extension modules
as you said
Travis Hance
@tjhance
May 22 2015 22:02
yeah I figured, was just clarifying :)
Marius Wachtler
@undingen
May 22 2015 22:30
@kmod I saw that you are mentioning in your last PR that you are seeing alot of conservative allocated mem for dicts: Did anyone yet find time to test the DenseMap approach we discussed recently?
If not I would try it out
Chris Toshok
@toshok
May 22 2015 22:30
i actually have a branch with that implemented
sec, let me push it :)
Marius Wachtler
@undingen
May 22 2015 22:31
did it help?
Chris Toshok
@toshok
May 22 2015 22:32
not tremendously.. i think for django-template.py it’s on the order of .6-.7 seconds
out of ~25
that has DenseMap/DenseSet/SmallVector implementations that take an AllocT
SmallVector usage might help if used more (I only changed callFunc and one place in ast_interpreter.cpp)
Chris Toshok
@toshok
May 22 2015 22:57
the unwinder is dead, long live the unwinder
Marius Wachtler
@undingen
May 22 2015 23:10
:+1: does anyone have perf numbers for it? or is perf comparison as long as libunwind does the linear search meaningless?
Chris Toshok
@toshok
May 22 2015 23:10
I ran django-template.py with it and it dropped the runtime from 25s to 16s
which is a pretty massive improvement
Marius Wachtler
@undingen
May 22 2015 23:10
cool :-)
Chris Toshok
@toshok
May 22 2015 23:10
and that’s without the binary search
Marius Wachtler
@undingen
May 22 2015 23:11
awesome
Chris Toshok
@toshok
May 22 2015 23:11
yeah, really impressive piece of work
maybe it’s time to backfill speed.pyston.org
so we can see a drop :)
oh wow, actually 16s was without the object cache i guess
real 0m14.858s
Chris Toshok
@toshok
May 22 2015 23:16
and perf reports 24% of that runtime is spent in the place that would benefit from the binary search, so we could see a >50% reduction in runtime with the new unwinder
Marius Wachtler
@undingen
May 22 2015 23:17
:-)
Travis Hance
@tjhance
May 22 2015 23:36
O: that's awesome!
Chris Toshok
@toshok
May 22 2015 23:40
what can cause jit object cache misses?
Travis Hance
@tjhance
May 22 2015 23:43
Next lets get an intern to write us a generational collector :)
toshok @toshok points at @rudi-c
Chris Toshok
@toshok
May 22 2015 23:44
cough
weird. i’m looking at the disassembly for boxInt, and the 0 <= n part of the if check is missing
oh i see, unsigned
if (0 <= n && n < 100) can be turned into if (n > 99)
Marius Wachtler
@undingen
May 22 2015 23:50
We had a few problems with phi nodes which got emitted in a non deterministic way
It could also be our ConstClassesPass I guess
but there is some crude code to help debug it: entry.cpp:290