980117 - (sccache-win) Implement sccache for Windows builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: sccache-win

Feel free to redirect review for some parts if you prefer. I'm flagging you because you have ccache knowledge and reviewed the first sccache implementation.

Note the patch is not against the existing version because there's too few in common for such a patch to be meaningful.

Note win32util.py is stolen from python/mozbuild/mozbuild/action/cl.py in m-c and makeutil.py from python/mozbuild/mozbuild/makeutil.py, so technically, they don't really need a review.

This is significantly more complex than the original implementation, but it was necessary to do so on Windows. It turns out to also improve performance on Linux builds.

Comment 2

[Mike Hommey [:glandium]]

Attachment: sccache-win

With a typo fix.

Comment 3

[Michael Shal [:mshal]]

Attachment: setup.sh

win64 setup.sh for tooltool

Comment 4

[Mike Hommey [:glandium]]

Attachment: Add some timing stats

Comment 5

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #2)
> Created attachment 8396229 [details] [diff] [review]
> sccache-win
> 
> With a typo fix.

Oops, this typo was in the original version, and is the cause for this failure:
   https://tbpl.mozilla.org/php/getParsedLog.php?id=36695961&tree=Try#error0

Comment 6

[Michael Shal [:mshal]]

Comment on attachment 8396457 [details]
setup.sh

setup.sh in tooltool -

sha512: 258c35efc786841267f8d377943274bd3eb4bd81bed2186db57b98c1543f4c513faf4f7845daf66821ef3b555ea3c2133e0d980a5ef10154f6d24596ff3a5de5

size: 95

Comment 7

[Chris AtLee [:catlee]]

Comment on attachment 8396229 [details] [diff] [review]
sccache-win

Review of attachment 8396229 [details] [diff] [review]:
-----------------------------------------------------------------

phew, that's a lot of code!

looks good overall, but I'm sure I don't understand everything. how much have you ironed out any threading/RPC issues in the client/server?

::: sccache/cache.py
@@ +35,5 @@
> +        '''
> +        if not self._data:
> +            data = StringIO()
> +            with gzip.GzipFile(mode='w', compresslevel=6, fileobj=data) as fh:
> +                fh.write(self._obj)

data = fh.read() would work too

::: sccache/sccache.py
@@ +84,5 @@
> +                client = CommandClient(('localhost', PORT))
> +                result = client.request(data)
> +            except socket.error as e:
> +                if e.errno == ECONNREFUSED: # Connection refused
> +                    raise RuntimeError("Couldn't start server")

do you really want to fail with an exception here? would it be better to just do the compilation without caching?

Comment 8

[Mike Hommey [:glandium]]

Thanks for the review.

(In reply to Chris AtLee [:catlee] from comment #7)
> Comment on attachment 8396229 [details] [diff] [review]
> sccache-win
> 
> Review of attachment 8396229 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> phew, that's a lot of code!
> 
> looks good overall, but I'm sure I don't understand everything. how much
> have you ironed out any threading/RPC issues in the client/server?

I originally had many. I'm now confident it has been tested enough to be deemed robust.

> ::: sccache/cache.py
> @@ +35,5 @@
> > +        '''
> > +        if not self._data:
> > +            data = StringIO()
> > +            with gzip.GzipFile(mode='w', compresslevel=6, fileobj=data) as fh:
> > +                fh.write(self._obj)
> 
> data = fh.read() would work too

Do you mean self._data = fh.read()? doesn't that require a fh.seek(0)?

> ::: sccache/sccache.py
> @@ +84,5 @@
> > +                client = CommandClient(('localhost', PORT))
> > +                result = client.request(data)
> > +            except socket.error as e:
> > +                if e.errno == ECONNREFUSED: # Connection refused
> > +                    raise RuntimeError("Couldn't start server")
> 
> do you really want to fail with an exception here? would it be better to
> just do the compilation without caching?

I'm kind of reluctant to let compilation work without cache without noticing.

Comment 9

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #8)
> > do you really want to fail with an exception here? would it be better to
> > just do the compilation without caching?
> 
> I'm kind of reluctant to let compilation work without cache without noticing.

That being said, there are other scenarios where that happens, too... mmmm...

Comment 10

[Mike Hommey [:glandium]]

Attachment: sccache.tar.xz

This is both sccache patches from this bug + boto, pydns and which from the older sccache from bug 940788, in a form for upload to tooltool.

Comment 11

[Justin Wood (:Callek)]

sccache.tar.xz of size 160232 uploaded with sha512sum of 
8656c3fc2daa66839ec81a0edbd9759040a83c7a41c3e472d7f90508b80eefd008b87305dc8549b4ff6098dc33fe17fedc9b4eb76cf5307d5f22dae925c033db

Comment 12

[Mike Hommey [:glandium]]

Attachment: Use sccache on win32 try builds

Comment 13

[Michael Shal [:mshal]]

Comment on attachment 8403175 [details] [diff] [review]
Use sccache on win32 try builds

>diff --git a/build/mozconfig.cache b/build/mozconfig.cache

>+        # Windows builds have a default wrapper that needs to be overridden
>+        mk_add_options "export CC_WRAPPER="
>+        mk_add_options "export CXX_WRAPPER="

Does it make sense to export these in wrapper.m4 instead? Presumably if we wanted to experiment with another --with-compiler-wrapper on Windows, we'd also want to disable the py_action stuff.

Comment 14

[Mike Hommey [:glandium]]

(In reply to Michael Shal [:mshal] from comment #13)
> Does it make sense to export these in wrapper.m4 instead? Presumably if we
> wanted to experiment with another --with-compiler-wrapper on Windows, we'd
> also want to disable the py_action stuff.

It does make sense, but it's kind of dangerous, in the sense that it would hide that you have to handle what the py_action stuff does in your own wrapper, and you wouldn't figure that out easily.

Comment 15

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80c78d69a500

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80c78d69a500