Skip to content

Conversation

@againull
Copy link
Contributor

Resetting command lists only in sycnhronization points caused
performance regression because we reuse less command lists and
new command lists are getting created more often which adds
performance overhead.

Return the old behavior back: reset signalled command lists
each time before requesting a command list to submit a command.

Resetting command lists only in sycnhronization points caused
performance regression because we reuse less command lists and
new command lists are getting created more often which adds
performance overhead.

Return the old behavior back: reset signalled command lists
each time before requesting a command list to submit a command.
@againull againull requested a review from a team as a code owner June 15, 2022 23:31
@againull againull requested a review from smaslov-intel June 15, 2022 23:31

// We need a command list to submit the command, that's why reset signalled
// command lists to avoid creating new one.
resetCommandLists(Queue);
Copy link
Contributor

Choose a reason for hiding this comment

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

this way you are going to check all running fences at each enqueue request whereas previously this check/reset was only done when no command-list was available. should we add this check/shortcut to resetCommandLists?

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 agree, thanks for noticing. Unfortunately, one problem with just adding check/shortcut to resetCommandLists is that available command list may be occupied by another thread as soon as we unlock the queue mutex, so when we reach getAvailableCommandList for the current thread there will be no available command list again.

That's why I returned the old behavior back - reset command lists in the getAvailableCommandLists. It is ok in terms of mutex locks now because unnecessary locks were removed from retain/release functions in b545c60

@againull againull requested a review from smaslov-intel June 27, 2022 23:09
@againull againull changed the title [SYCL] Reset signalled command lists before requesting one [SYCL] Reset signalled command list if there no available command list Jun 27, 2022
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

LGTM

@againull againull merged commit 8a4777d into intel:sycl Jun 28, 2022
@againull againull deleted the fix_perf_regression branch December 3, 2022 00:12
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