RFR: 8295661: CompileTask::compile_id() should be passed as int
Damon Fenacci
duke at openjdk.org
Fri Dec 16 09:39:19 UTC 2022
On Mon, 12 Dec 2022 18:50:40 GMT, Tom Rodriguez <never at openjdk.org> wrote:
>> Changed return type of `CompileTask::compile_id()` from `int` to `uint`.
>> Also modified the type everywhere `compile_id` is used (parameters, local variables, string formatting, constant assignments) *as much as possible*.
>> Added *asserts* to check for valid value range where not possible.
>
> Why isn't the proper fix to simply change the `CompileTask::_compile_id` to an int? Every other piece of code thinks it's an int so we should that declaration be controlling? `CompileBroker::assign_compile_id` returns int which seems much more definitive to me. Is there some known problem with running out of compile ids in the in signed range?
>
> If this range is really being promoted to unsigned int then I think this needs to be properly exposed all the way through the JVMCI API with some new API to expose it as a long. Simply adding asserts for something we believe can legally occur really isn't sufficient.
@tkrodriguez thanks a lot for your insight. I see your point: the pragmatic approach you propose is surely more appropriate. Reverting the change.
@tkrodriguez @TobiHartmann @dougxc I've reverted the previous changes and changed `CompileTask::_compile_id` to an `int` instead. I've also changed the type of the compile id from `uint` to `int` in a few other places to make things consistent.
> If we were really worried about overflow then I think it should be promoted to a jlong since the difference between the signed and unsigned range is really relatively small. I think the required changes to JVMCI could be made in a backward compatible way where there is new API treats it as long. The primary place where this is exposed is in `HotSpotCompilationRequest.getId`. The old API could be made to throw an exception if it encounters a too large id. Anyway if we want to address that issue I can probably provide the required JVMCI changes.
Thanks @tkrodriguez @TobiHartmann @dougxc for the reviews!
-------------
PR: https://git.openjdk.org/jdk/pull/11630
More information about the hotspot-dev
mailing list