Closed Bug 980117 (sccache-win) Opened 6 years ago Closed 6 years ago

Implement sccache for Windows builds

Categories

(Firefox Build System :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

No description provided.
Depends on: 980120
Depends on: 982283
Depends on: 985263
Depends on: 985727
Depends on: 985833
Depends on: 985835
Depends on: 985836
Depends on: 987535
Depends on: 987537
Attached patch sccache-win (obsolete) — Splinter Review
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.
Attachment #8396228 - Flags: review?(catlee)
Attached patch sccache-winSplinter Review
With a typo fix.
Attachment #8396228 - Attachment is obsolete: true
Attachment #8396228 - Flags: review?(catlee)
Attachment #8396229 - Flags: review?(catlee)
Attachment #8396229 - Attachment is patch: true
Attachment #8396229 - Attachment mime type: text/x-patch → text/plain
Attached file setup.sh
win64 setup.sh for tooltool
Attachment #8396457 - Flags: review?(rail)
Attachment #8396457 - Flags: review?(mh+mozilla)
Attachment #8396457 - Flags: review?(rail) → review+
Attachment #8396823 - Flags: review?(catlee)
(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
Attachment #8396457 - Flags: review?(mh+mozilla) → review+
Depends on: 988722
Comment on attachment 8396457 [details]
setup.sh

setup.sh in tooltool -

sha512: 258c35efc786841267f8d377943274bd3eb4bd81bed2186db57b98c1543f4c513faf4f7845daf66821ef3b555ea3c2133e0d980a5ef10154f6d24596ff3a5de5

size: 95
Attachment #8396457 - Flags: checkin+
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?
Attachment #8396229 - Flags: review?(catlee) → review+
Attachment #8396823 - Flags: review?(catlee) → review+
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.
(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...
Attached file 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.
Attachment #8398209 - Flags: checkin?(bugspam.Callek)
Depends on: 989147
sccache.tar.xz of size 160232 uploaded with sha512sum of 
8656c3fc2daa66839ec81a0edbd9759040a83c7a41c3e472d7f90508b80eefd008b87305dc8549b4ff6098dc33fe17fedc9b4eb76cf5307d5f22dae925c033db
Attachment #8398209 - Flags: checkin?(bugspam.Callek) → checkin+
No longer depends on: 985263
Depends on: 993331
Attachment #8403175 - Flags: review?(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.
Attachment #8403175 - Flags: review?(mshal) → review+
(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.
Depends on: 993728
https://hg.mozilla.org/mozilla-central/rev/80c78d69a500
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Blocks: 1000713
Depends on: 1001517
Blocks: 1008015
Depends on: 1010316
Depends on: 1024651
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.