RFR: JDK-8136385: Various build speed improvements for windows

[Erik Joelsson]

On 2015-09-16 11:37, Magnus Ihse Bursie wrote:
> On 2015-09-11 16:46, Erik Joelsson wrote:
>> Hello,
>>
>> In the build-infra project, I have made various minor build speed 
>> improvements for Windows. These mostly affect incremental build 
>> performance, but in certain cases normal builds are also greatly 
>> affected. I find these worth committing to JDK 9 separately.
>>
>> List of improvements:
>>
>> * Rewrote DependOnVariable to use "-include" instead of $(shell 
>> $(CAT) ...) to read the old variable value from the last make 
>> invocation. This has a pretty big impact on incremental build 
>> performance on Windows. Also started using the file function in make 
>> when available (in make 4.0) instead of $(shell $(PRINTF) ...) when 
>> writing these files.
>>
>> * Implemented ListPathsSafely using the file function when available. 
>> Since we require make 4.0 in cygwin, this will usually be the case 
>> when it matters.
>>
>> * Reduced the number of shell execution in the compiler and link 
>> recipes by joining them together with "; \". When compiling a lot of 
>> native code, this tends to get quite expensive.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8136385
>> Webrev: http://cr.openjdk.java.net/~erikj/8136385/webrev.01/
>
> As a general comment, I've experimented a bit with using .ONESHELL, 
> which will automatically concatenate separate lines in a recipe and 
> execute them by a single shell. When it works, it improves performance 
> significantly, especially in Windows. Having it enabled all over the 
> board would also let us avoid those "&& \" constructs.
>
> However, a lot of recipes needs to be updated to work properly with 
> .ONESHELL. Also, it was also introduced in GNU Make 3.82 so we would 
> need to bump our lowest supported platform number. Nevertheless, I 
> think it is the proper way forward, but it will take some time. So 
> changes such as this will be needed in the meantime.
>
I agree that .ONESHELL would be preferable. But as you said, 3.82 would 
require a bump in our required make version and at least the Ubuntu I'm 
using still has 3.81 as default.
> I have a few questions:
>
> In JavaCompilation.gmk, you replace $1_GREP_INCLUDE_OUTPUT := with 
> $1_GREP_INCLUDE_OUTPUT =. Any specific reason to drop the : or just a 
> typo? (And so also for $1_GREP_EXCLUDE_OUTPUT)
>
That was indeed intentional. Note that GREP_EXCLUDE_OUTPUT was already 
declared using = so unifying is one reason. More importantly though, the 
old ListPathsSafely only generated command lines for use in a recipe, 
which made it safe to evaluate at declaration time. The new 
ListPathsSafely evalues the writing to file as a make function, and as 
such, we want to delay that evaluation until recipe execution time.
> The ListPathsSafely function is really horrible and hard to read. :-/ 
> (I'm not blaming you; the version using "file"  is excellent). I have 
> a hard time figuring out that the legacy version (without "file") is 
> still correct. Maybe we should have a test for it?
>
I agree it needs tests. On the other hand, the build would most likely 
fail if it wasn't accurate. I did contemplate writing a test, but it 
seems like a lot of work.
> The indentation on DependOnVariableHelper looks weird. It might be the 
> patch but I'm not sure.
>
It was a bit weird. I had skipped indenting for the strip call. Fixed in 
new webrev.
> In NativeCompilation, I'm trying to figure out how the "ifneq 
> ($$($1_$2_DEP),)" is supposed to work. This is old code and I wouldn't 
> have reacted to it if it were not for the fact that you removed this 
> checked for the microsoft toolchain path. As I can see, the $1_$2_DEP 
> variable will be empty for .s files. But if we're compiling .s files 
> in solstudio, we'll still end up calling "$$($1_$2_COMP) 
> $$($1_$2_FLAGS) $$($1_$2_DEP_FLAG) $$($1_$2_DEP) ...". How could that 
> possibly work? And what happens for the microsoft case with your 
> patch? Now we're going to do the "$(SED) 
> $(DEPENDENCY_TARGET_SED_PATTERN) $$($1_$2_DEP) > 
> $$($1_$2_DEP_TARGETS)" operation, which we previously didn't do. Do we 
> actually compile any .s files? I can't understand how that would work. 
> *checking* No, we don't. In the jdk project, there's just a single .s 
> file 
> (./jdk/src/java.desktop/unix/native/libawt/awt/medialib/mlib_v_ImageCopy_blk.s) 
> and it's excluded from compilation. On the other hand, there *are* .s 
> files in the Hotspot project which we will need to be able to handle 
> at some point. Urgh. Messy. You'll have to decide if you want to clean 
> this up.
>
I figured this would work since we don't have .s files on Windows. 
Looking closer it also seems like .s files would be broken for other 
platforms. Not an issue in current JDK 9 though so I won't try to fix it 
in this patch as I couldn't verify the fix anyway.

New webrev:

Also, I looked at the schedule for the JEP 243 integration and it won't 
hit the hotspot forests until October, so I think we should go ahead 
with this change to ListPathsSafely in jdk9-dev now.

/Erik

/Erik