Request for Review (XXL): 6989984: Use standard include model for Hotspot

[Stefan Karlsson]

Hi Volker,

On 2010-11-19 19:02, Volker Simonis wrote:
> Hi Stefan and Bengt,
>
> I have some comments/questions regarding your change:
>
> - how did you create the "src/share/vm/precompiled.hpp". I have the
> impression that it contains much fewer lines compared to the old,
> generated file?

We took the old, generated file and removed the platform dependent 
files. They are still implicitly included by the other header files. We 
also added some ifdefs to handle the different build targets.

> - what is the policy of maintaining "src/share/vm/precompiled.hpp",
> i.e. when should a new include statement be added just into the
> requiring .cpp file and when will it be appropriate to add it to
> precompiled.hpp.

Currently, there's no policy for updating this file. We should only 
update this file when trying to optimize the build times. Adding a new 
header to any of the other header files will probably add it to the 
precompiled header. MakeDeps added almost all header files to 
precompiled.hpp, so this is not that different. The precompiled.hpp 
could probably be tweaked for even better build performance than we had 
before this change.

> - did you check the build performance with your changes on the various
> platforms? Are there any regressions?

We measured the build times on linux and windows and saw no performance 
regressions. Also, remember that we don't use precompiled headers when 
building with the Sun Studio compiler.

> And please, please synchronize this change with other changes,
> especially the other big ones like Coleens Symtable change to avoid
> merges as described here:
>
> http://old.nabble.com/Request-for-review-%28XL%29-6990754%3A-Use-native-memory-and-reference-counting-to-implement-SymbolTable-tt30138063.html#a30138063
>
> Please talk to Coleen who proposed that "..we could arrange that these
> symbol changes are pushed to hotspot-rt, run through the nightly tests
> and then pushed to hotspot repository before the other baselines.
> Maybe it should rate it's own integration slot.  I won't create merge
> change sets before pushing."

Yes, we're currently discussing how this should be pushed.

> Your change patch for example already doesn't run with the current
> hotspot-rt because change  "6970683: improvements to hs_err output"
> which was already pushed 4 weeks ago added a call  to
> VMError::fatal_error_in_progress() to parallelScavengeHeap.cpp which
> isn't reflected by your change (i.e. VMError isn't visible). So if you
> would push your change right now, this would result in a non-empty,
> potentially quite large merge set. So please apply one of the
> techniques mentioned in the thread referenced above to avoid such
> merges.

We will not push the patch as it is. The include statements are 
generated by a tool, which we'll run against the head of hotspot-rt 
before pushing our changes.

Thanks,
Stefan

> Thank you and best regards,
> Volker
>
> On Wed, Nov 17, 2010 at 4:57 PM, Stefan Karlsson
> <stefan.karlsson at oracle.com>  wrote:
>> We are simplifying the Hotspot build process by standardizing the code and
>> reducing the number of required tools. As a first step we are removing the
>> use of the MakeDeps tool and the includeDB files.
>>
>> This will help new developers get up to speed with the code, which will
>> benefit both internal developers and the open source community. It will also
>> make IDE integration simpler.
>>
>> MakeDeps provided these services:
>> * Includes between source files
>> * Platform dispatching
>> * Precompiled header management
>> * Makefile rules for source file dependencies
>> * Source files selection based on compiled target (compiler1, tiered,
>> kernel, ...)
>> * VisualStudio project creation
>> * Circular dependency detection
>>
>> To replace MakeDeps we updated all source files with explicit include
>> statements, include guards and platform dispatching with #ifdefs. We also
>> updated the makefiles to provide the rest of the MakeDeps services.
>>
>> Note: this change will update almost all source files and many makefiles.
>> The top-level make commands have not been changed.
>>
>> Webrev is available here:
>> http://cr.openjdk.java.net/~stefank/6989984.2/
>>
>> Bug:
>> 6989984: Use standard include model for Hotspot
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6989984
>>
>> Thanks,
>> Stefan and Bengt
>>