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

[Magnus Ihse Bursie]

On 2015-09-24 14:31, Erik Joelsson wrote:
>
>
> 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.

Okay.

>> 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.

It probably is, yes. I'll let you of the hook for this one, and maybe 
I'll add one as a follow-up exercise. :)

>> 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.
Thanks!

>> 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.
Good point.

>
> New webrev:

Looks good to me now.

/Magnus

>
> 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