RFR: 8191860: Add perfData.inline.hpp

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8191860: Add perfData.inline.hpp

Stefan Karlsson
Hi all,

Please review this patch to create a perfData.inline.hpp file and move
inline functions in perfData.hpp that depend on functions in other
.inline.hpp files.

http://cr.openjdk.java.net/~stefank/8191860/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8191860

Note 1: I consider growableArray.hpp to be an .inline.hpp in disguise,
since it includes allocation.inline.hpp.

Note 2: Some .hpp files that used to include perfData.hpp now explicitly
includes allocation.inline.hpp. We should deal with that, but in another
RFE.

thanks,
StefanK
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191860: Add perfData.inline.hpp

Erik Helin-2
On 11/24/2017 02:21 PM, Stefan Karlsson wrote:
> Hi all,
>
> Please review this patch to create a perfData.inline.hpp file and move
> inline functions in perfData.hpp that depend on functions in other
> .inline.hpp files.
>
> http://cr.openjdk.java.net/~stefank/8191860/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8191860

Looks good, Reviewed. Thanks for cleaning this up.

Thanks,
Erik

> Note 1: I consider growableArray.hpp to be an .inline.hpp in disguise,
> since it includes allocation.inline.hpp.
>
> Note 2: Some .hpp files that used to include perfData.hpp now explicitly
> includes allocation.inline.hpp. We should deal with that, but in another
> RFE.
>
> thanks,
> StefanK
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191860: Add perfData.inline.hpp

Per Liden
In reply to this post by Stefan Karlsson
Looks good!

/Per

On 2017-11-24 14:21, Stefan Karlsson wrote:

> Hi all,
>
> Please review this patch to create a perfData.inline.hpp file and move
> inline functions in perfData.hpp that depend on functions in other
> .inline.hpp files.
>
> http://cr.openjdk.java.net/~stefank/8191860/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8191860
>
> Note 1: I consider growableArray.hpp to be an .inline.hpp in disguise,
> since it includes allocation.inline.hpp.
>
> Note 2: Some .hpp files that used to include perfData.hpp now explicitly
> includes allocation.inline.hpp. We should deal with that, but in another
> RFE.
>
> thanks,
> StefanK
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191860: Add perfData.inline.hpp

Stefan Karlsson
In reply to this post by Erik Helin-2
Thanks, Erik!

StefanK

On 2017-11-24 14:46, Erik Helin wrote:

> On 11/24/2017 02:21 PM, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to create a perfData.inline.hpp file and move
>> inline functions in perfData.hpp that depend on functions in other
>> .inline.hpp files.
>>
>> http://cr.openjdk.java.net/~stefank/8191860/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8191860
>
> Looks good, Reviewed. Thanks for cleaning this up.
>
> Thanks,
> Erik
>
>> Note 1: I consider growableArray.hpp to be an .inline.hpp in disguise,
>> since it includes allocation.inline.hpp.
>>
>> Note 2: Some .hpp files that used to include perfData.hpp now
>> explicitly includes allocation.inline.hpp. We should deal with that,
>> but in another RFE.
>>
>> thanks,
>> StefanK
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191860: Add perfData.inline.hpp

Stefan Karlsson
In reply to this post by Per Liden
Thanks, Per!

StefanK

On 2017-11-24 15:06, Per Liden wrote:

> Looks good!
>
> /Per
>
> On 2017-11-24 14:21, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to create a perfData.inline.hpp file and move
>> inline functions in perfData.hpp that depend on functions in other
>> .inline.hpp files.
>>
>> http://cr.openjdk.java.net/~stefank/8191860/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8191860
>>
>> Note 1: I consider growableArray.hpp to be an .inline.hpp in disguise,
>> since it includes allocation.inline.hpp.
>>
>> Note 2: Some .hpp files that used to include perfData.hpp now explicitly
>> includes allocation.inline.hpp. We should deal with that, but in another
>> RFE.
>>
>> thanks,
>> StefanK
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191860: Add perfData.inline.hpp

David Holmes
In reply to this post by Stefan Karlsson
Hi Stefan,

On 24/11/2017 11:21 PM, Stefan Karlsson wrote:

> Hi all,
>
> Please review this patch to create a perfData.inline.hpp file and move
> inline functions in perfData.hpp that depend on functions in other
> .inline.hpp files.
>
> http://cr.openjdk.java.net/~stefank/8191860/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8191860
>
> Note 1: I consider growableArray.hpp to be an .inline.hpp in disguise,
> since it includes allocation.inline.hpp.

Is that why you didn't #include it instead of forward declaring:

+ template <typename T> class GrowableArray;

?

> Note 2: Some .hpp files that used to include perfData.hpp now explicitly
> includes allocation.inline.hpp. We should deal with that, but in another
> RFE.

Yes we should deal with that - but the fanout from these changes can get
huge. I don't relish trying to refactor arguments.hpp into
arguments.inline.hpp for example.

Cheers,
David

> thanks,
> StefanK
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191860: Add perfData.inline.hpp

Stefan Karlsson
Hi David,

On 2017-11-29 11:11, David Holmes wrote:

> Hi Stefan,
>
> On 24/11/2017 11:21 PM, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to create a perfData.inline.hpp file and move
>> inline functions in perfData.hpp that depend on functions in other
>> .inline.hpp files.
>>
>> http://cr.openjdk.java.net/~stefank/8191860/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8191860
>>
>> Note 1: I consider growableArray.hpp to be an .inline.hpp in disguise,
>> since it includes allocation.inline.hpp.
>
> Is that why you didn't #include it instead of forward declaring:
>
> + template <typename T> class GrowableArray;
>
> ?

Yes, this was the initial motivation.

However, I do think its nice to only forward declare classes, where it's
possible, so that we don't have to bring in dependencies against other
header files unnecessarily. This is extra important given that a lot of
our header files transitively include more than they need.

>
>> Note 2: Some .hpp files that used to include perfData.hpp now
>> explicitly includes allocation.inline.hpp. We should deal with that,
>> but in another RFE.
>
> Yes we should deal with that - but the fanout from these changes can get
> huge. I don't relish trying to refactor arguments.hpp into
> arguments.inline.hpp for example.


I've created a RFE and a set of patches to do this clean-up:
https://bugs.openjdk.java.net/browse/JDK-8192061

I'll send it out shortly.

Thanks,
StefanK

>
> Cheers,
> David
>
>> thanks,
>> StefanK