Skip to content

Conversation

@lslusarczyk
Copy link
Contributor

@lslusarczyk lslusarczyk commented Nov 19, 2025

[functional change]: GlobalHandler instance move from a static variable declared inside a method to global scope. This changes when the first object is created. Previously it would only be created if getInstancePtr was called, now it happens at the time of loading the library. Users pay for the instance even if they never use the global handle.
Motivation is performance - accessing global variable declared inside a function can be slower than to variable in global scope. Explained e.g. here

[non-functional-change]: various functions exising on hot submit-kernel path get inlined

This PR is part of #20668 which was split into two. See plots in that PR desciption for performance comparisons.

bool GlobalHandler::isInstanceAlive() {
return GlobalHandler::getInstancePtr();
}
GlobalHandler *GlobalHandler::RTGlobalObjHandler = new GlobalHandler();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this was meant to be NFC changes, but technically this changes when the first object is created. Previously it would only be created if getInstancePtr was called, now it happens at the time of loading the library. I don't think that's necessarily a problem, but so far it is the only functional change I have noticed in this PR and technically it makes users pay for the instance even if they never use the global handler (implicitly). That said, it is hard to think of such a case.

Copy link
Contributor Author

@lslusarczyk lslusarczyk Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is on purpose to avoid slower access to static variables declared inside a method as we discussed in this comment of original PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly against doing this, but I wonder if it would be cleaner to do it in a separate PR and mark this "[NFCI]". That way, if there are unforeseen issues with doing the initialization earlier, it should hopefully be easier to isolate the cause when bisecting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated summary to not miss this change when doing "git log" later. Can we leave as it? All CI already passed.
Hopefully work of extracting this non-NFC change will not be have to be paid at all. Risk is tiny I think. In case of problems we will pay - I will revert, extract & reapply part.

@lslusarczyk lslusarczyk force-pushed the enqueueImpKernel_inline branch from 08e1487 to baa17a7 Compare November 20, 2025 10:58
@lslusarczyk lslusarczyk changed the title [SYCL] optimize enqueueImpKernel by inlining [SYCL] optimize enqueueImpKernel, GlobalHandler instance move to global scope, add various inline Nov 20, 2025
@lslusarczyk lslusarczyk changed the title [SYCL] optimize enqueueImpKernel, GlobalHandler instance move to global scope, add various inline [SYCL] optimize enqueueImpKernel by inlining Nov 20, 2025
@steffenlarsen steffenlarsen merged commit 717967c into intel:sycl Nov 20, 2025
48 of 50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants