RFR [M] : 8087333, Optionally Pre-Generate the HotSpot Template Interpreter

[Bertrand Delsart]

Thanks David,

Further trimming, keeping only the open questions and action items.

In addition, I had to revert back some of what I have undone in webrev.01.

Removing the %.S rule did not work as expected on all targets. gcc 
handles differently 's' and 'S' files, doing the preprocessing only for 
the 'S' extension. Hence I added back the %.S rules but without all the 
C/C++ related FLAGS. The %.S rule is the same as the %.s rules (except 
for the printed message) but gcc does something different thanks to the 
effective file extension.

Incremental open diff:

+# Copyright (c) 2003, 2015, Oracle and/or its affiliates. All rights 
reserved.
  # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  #
  # This code is free software; you can redistribute it and/or modify it
@@ -170,6 +170,12 @@
  	$(QUIETLY) $(REMOVE_TARGET)
  	$(QUIETLY) $(AS.S) $(DEPFLAGS) -o $@ $< $(COMPILE_DONE)

+# gcc applies preprocessing if the file extension is .S instead of .s
+%.o: %.S
+	@echo $(LOG_INFO) Preprocessing and assembling $<
+	$(QUIETLY) $(REMOVE_TARGET)
+	$(QUIETLY) $(AS.S) $(DEPFLAGS) -o $@ $< $(COMPILE_DONE)
+
  %.s: %.cpp
  	@echo $(LOG_INFO) Generating assembly for $<
  	$(QUIETLY) $(GENASM.CXX) -o $@ $<


On 19/06/2015 09:38, David Holmes wrote:
>>> src/share/vm/precompiled/precompiled.hpp
>>>
>>> Seems okay - but of course begs the question whether this has been
>>> checked with PCH disabled?
>>
>> It has been tested but maybe not for the latest version. It does pass
>> JPRT but I'm not sure of whether JPRT performs both PCH and non-PCH
>> builds (if not, it should :-))
>
> Yes it should but it doesn't. :)

This may explain why it is already broken in hs-rt for some closed 
platforms :-)

Good news is that non PCH builds look OK on the non-closed platforms I 
tested before noticing the closed issue.

Note sure the closed platforms are worth fixing in this CR since I saw 
errors in files that have not been touched for a long time. It probably 
means nobody is doing non-PCH builds for these platforms.

>>> src/share/vm/runtime/arguments.hpp
>>>
>>> I'm getting doubtful about the numerous, and varied "friend"
>>> declarations. Why should CodeCacheExtensions need anything but the
>>> public API of Arguments ?
>>
>> Because CodeCacheExtensions may include ergonomics, some of them being
>> more complex than just changing one flag.
>>
>> As an example, my extension may have to force interpreter mode and the
>> cleanest and most robust way of doing it is through a non public API:
>>
>>      Arguments::set_mode_flags(Arguments::_int);
>>
>> Are you willing to open that API to every hotspot component ?
>>
>> There is always a trade-off between opening a few calls to everybody and
>> opening all of them to selected friends we can watch closely. I'd rather
>> keep the friend declaration but I'm biased since it makes it way easier
>> for me :-)
>
> I guess I just don't like "secret societies" :) Whenever a friend gets
> introduced it always begs the questions: does this class really need to
> access internals? If so, does that suggest the need for a more extensive
> public API? I didn't (and still don't) have enough context to answer
> that here.

I did add (or enhance) a few public APIs in a several classes, when it 
looked safe. However, there are calls that I'm reluctant to open with no 
control over who/when/how they would be used in production.

I checked that set_mode_flags() is (currently) the only one I need in
Arguments.

Hence we are back to my initial question:
 >> Are you willing to open that API to every hotspot component ?

I'm reluctant to do it but I will not object. set_mode_flags is called 
from different locations as a way to guarantee some coherence (this is 
why I'm calling it). However, if opened widely, there is for instance 
the risk of someone thinking it can be used to safely to activate the 
compilers late during arguments marshalling. Past a given point, this is 
no longer safe.

A "friend" declaration is not much better when it opens doors to a 
friend class used regularly in production (unless controlled by the team 
owning the opened class). However, this should not be the case for the 
friend declarations I added. First, they are limited to friend classes 
that are unused in openjdk and that are also empty in most of our 
deployed targets. In addition, Oracle's version of CodeCacheExtension is 
owned by runtime, meaning the friend class is part of the same team as 
the opened class. We cannot control what others might do with this 
privilege but this is the responsibility of whoever would implement 
another CodeCacheExtension.

Knowing that, pick your choice: either open set_mode_flags (and maybe 
others later) or keep the friend declaration.

Thanks,

Bertrand.


-- 
Bertrand Delsart,                     Grenoble Engineering Center
Oracle,         180 av. de l'Europe,          ZIRST de Montbonnot
38330 Montbonnot Saint Martin,                             FRANCE
bertrand.delsart at oracle.com             Phone : +33 4 76 18 81 23

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged
information. Any unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient,
please contact the sender by reply email and destroy all copies of
the original message.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~