1443230 - Keep hashtable of interface indexes in xpt.py

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

I looked at a profile of running xpt.py on a final link that includes all XPT files. This was for a variant that outputs C++ instead of an XPT file, but I think the performance should be similar. The base time was 2.7 seconds.

I noticed a lot of the running time was spent calling a list index, which is done to find the index of an interface. Replacing these index calls with a map from interfaces to their indexes reduced the running time to 1 second.

There are also some calls to |interface in self.interfaces| in _sanityCheck which can also be replaced by using this same index map. That reduced the running time further to 0.9 seconds.

This does make the code more fragile, because the map has to be updated in parallel with the list, but it doesn't seem too bad to me.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1443230 - Keep hashtable of interface indexes in xpt.py.

A lot of time is spent during the final big XPT link determining what
the index is for each interface.  Changing this to use a map
eliminates about 2/3 of the running time. This patch reduces the run
time to a little under a second on my local OSX machine.

Review commit: https://reviewboard.mozilla.org/r/225118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225118/

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8956213 [details]
Bug 1443230 - Keep hashtable of interface indexes in xpt.py.

https://reviewboard.mozilla.org/r/225118/#review231208

I think I'd prefer a wrapping type for interfaces, that pretends to be the list type it replaces, rather than relying on callers doing the right thing.

Something like:

class IndexedList(object):
    def __init__(self, iterable):
        self._list = []
        self._index_map = {}
        for i in iterable:
            self.append(i)

    def sort(self):
        self._list.sort()
        self._index_map = {val: i for i, val in enumerate(self._list, 1)}

    def append(self, val):
        self._index_map[val] = len(self._list)]
        self._list.append(val)

    def index(self, what):
        return self._index_map[what]

    def __contains__(self, what):
        return what in self._index_map

    def __iter__(self):
        return iter(self._list)

    def __getitem__(self, index):
        return self._list[index]

    def __len__(self):
        return len(self._list)

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Mike Hommey [:glandium] from comment #2)
> I think I'd prefer a wrapping type for interfaces, that pretends to be the
> list type it replaces, rather than relying on callers doing the right thing.

Oh, wow, that's much better.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8956213 [details]
Bug 1443230 - Keep hashtable of interface indexes in xpt.py.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225118/diff/1-2/

Comment 5

[Andrew McCreight [:mccr8]]

I confirmed locally that I saw the same performance improvements in a regular build without my XPT patches. 

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a116128b75b03bc81aa011f8cf0ae8e34985da2

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8956213 [details]
Bug 1443230 - Keep hashtable of interface indexes in xpt.py.

https://reviewboard.mozilla.org/r/225118/#review231440

::: xpcom/typelib/xpt/tools/xpt.py:112
(Diff revision 2)
> +    def __init__(self, iterable):
> +        self._list = []
> +        self._index_map = {}
> +        for i in iterable:
> +            self.append(i)
> +        self._generate_index_map()

You don't need this, since you're going through append() to add the contents of iterable, so the index map will already be filled for each elements in there.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Mike Hommey [:glandium] from comment #6)
> You don't need this, since you're going through append() to add the contents
> of iterable, so the index map will already be filled for each elements in
> there.

Oops, right. I'm not sure why I didn't see that.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8956213 [details]
Bug 1443230 - Keep hashtable of interface indexes in xpt.py.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225118/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08802a2ae2b5
Keep hashtable of interface indexes in xpt.py. r=glandium

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/08802a2ae2b5