Skip to content

[SYCL] Release commands with no dependencies after they're enqueued #2492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 15 commits into from
Oct 1, 2020
Merged

[SYCL] Release commands with no dependencies after they're enqueued #2492

merged 15 commits into from
Oct 1, 2020

Conversation

cperkinsintel
Copy link
Contributor

Taking a simpler approach as suggested by @sergey-semenov . Presently, commands that do not have memory dependencies are only released if .wait() is called. If it is not, several resources (event, queue, command, kernel) are held onto.
This fix deletes stores that command event in the USMEvents ( so it is 'owned') and then deletes the command itself. Added a lit test to verify resource release.

@cperkinsintel
when a command has no mem dependencies, we now release it early, but …
0a3ecf3 
…track its event in USMEvents, guaranteeing both its completion and ultimate release

Signed-off-by: Chris Perkins <[email protected]>
@cperkinsintel
added lit test
11161ca 
Signed-off-by: Chris Perkins <[email protected]>
@cperkinsintel cperkinsintel requested a review from a team as a code owner September 17, 2020 23:53
@cperkinsintel cperkinsintel mentioned this pull request Sep 17, 2020
Closed
sergey-semenov
sergey-semenov reviewed Sep 18, 2020
View reviewed changes
Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

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

Minor: I think something like "Release commands with no dependencies after they're enqueued" would be a more descriptive PR title.

sergey-semenov
sergey-semenov reviewed Sep 18, 2020
View reviewed changes
romanovvlad
romanovvlad reviewed Sep 18, 2020
View reviewed changes
@cperkinsintel
review feedback
53fffa0 
Signed-off-by: Chris Perkins <[email protected]>
@cperkinsintel cperkinsintel changed the title [SYCL] resources released even when no .wait() [SYCL] Release commands with no dependencies after they're enqueued Sep 18, 2020
@cperkinsintel
re-run ci
8895569 
Signed-off-by: Chris Perkins <[email protected]>
@cperkinsintel
Copy link
Contributor Author

I am glad to see that changing the capitalization of one word in a comment has caused the buildbot linux test to now pass, but the Windows one to now fail.

@cperkinsintel
re-run ci?
995a8d2 
Signed-off-by: Chris Perkins <[email protected]>
@cperkinsintel
Copy link
Contributor Author

cperkinsintel commented Sep 21, 2020 •
edited
Loading

@sergey-semenov @romanovvlad - Last week I made the changes you wanted, and the tests have passed. So this should be ready to merge.

HOWEVER, as you can see from the commit logs, the requested changes made on Friday were no big deal, yet the tests failed afterwards. Since then I've merely updated the comments twice. Once resulted in different tests failing, and another in tests passing.

So, do we have a races or other indeterminate test failures that you already know about, and therefore it's fine to merge? Or, is this PR possibly introducing a race that will lead to random failures?

FWIW, I have not produced test failures on my own dev environment. Though, I am not repeating runs.

@romanovvlad
Copy link
Contributor

@sergey-semenov @romanovvlad - Last week I made the changes you wanted, and the tests have passed. So this should be ready to merge.

HOWEVER, as you can see from the commit logs, the requested changes made on Friday were no big deal, yet the tests failed afterwards. Since then I've merely updated the comments twice. Once resulted in different tests failing, and another in tests passing.

So, do we have a races or other indeterminate test failures that you already know about, and therefore it's fine to merge? Or, is this PR possibly introducing a race that will lead to random failures?

FWIW, I have not produced test failures on my own dev environment. Though, I am not repeating runs.

Hm, I do not see these fails in other PRs. I think it can be a sporadic problem which being introduced by this PR.
@cperkinsintel Could you please double check that the patch is OK locally?

@cperkinsintel
Copy link
Contributor Author

@romanovvlad - I was able to get a failure last night. In one of the interop tests, IIRC. I'm not sure this is causing the failure, or merely aggravating it, but it seems problematic. I'll start investigating. If anyone has any ideas on possible culprits, let me know.

romanovvlad
romanovvlad reviewed Sep 24, 2020
View reviewed changes
s-kanaev
s-kanaev reviewed Sep 24, 2020
View reviewed changes
@cperkinsintel
ci test
99d25a0 
Signed-off-by: Chris Perkins <[email protected]>
@cperkinsintel
ci test
0ac6f41 
Signed-off-by: Chris Perkins <[email protected]>
@cperkinsintel
verify
e583304 
Signed-off-by: Chris Perkins <[email protected]>
sergey-semenov
sergey-semenov previously approved these changes Sep 29, 2020
View reviewed changes
Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

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

LGTM

sergey-semenov
sergey-semenov previously approved these changes Sep 30, 2020
View reviewed changes
@cperkinsintel
comment update
349b81d 
Signed-off-by: Chris Perkins <[email protected]>
@bader bader merged commit 4785f47 into intel:sycl Oct 1, 2020
@sergey-semenov sergey-semenov mentioned this pull request Oct 1, 2020
Closed
@bader bader mentioned this pull request Oct 2, 2020
Merged
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Oct 5, 2020
Merge remote-tracking branch 'upstream/sycl' into global_constructors…
e4b7866 
…_wrapper

* upstream/sycl: (1021 commits)
  [SYCL] Enable async_work_group_copy for scalar and vector bool types (intel#2582)
  [SYCL] Fix element type in handler::copy (intel#2590)
  [NFC][SYCL] Remove unnecessary if condition (intel#2585)
  [SYCL][NFC] Fix SYCL lit test execution on a system w/o GPU (intel#2584)
  [SYCL] Add error handling for non-uniform work group size case (intel#2569)
  [SYCL][ESIMD] Preserve undef initializer for globals in ESIMDLowerVecArg pass (intel#2555)
  [SYCL] Make Level-Zero events visible on the host (intel#2576)
  [Driver][SYCL][NFC] Add help information for -Wno-sycl-strict (intel#2570)
  [SYCL] Relax test to work in Win32 environment. (intel#2580)
  [SYCL] Emit suppressed warnings from SYCL headers (intel#2575)
  [SYCL][NFC] Cover more classes with ABI tests (intel#2577)
  [SYCL][ESIMD] Update ESIMD tests and add raw send support. (intel#2482)
  [SYCL] Make ESIMD on-device tests require linux,gpu,opencl. (intel#2560)
  [SYCL] Release commands with no dependencies after they're enqueued (intel#2492)
  [SYCL] Add multi-device and multi-platform support for SYCL_DEVICE_ALLOWLIST  (intel#2483)
  [SYCL] Try to enqueue host command depencies (intel#2561)
  [SYCL][ESIMD][NFC] Align namespace name with the spec guidelines (intel#2573)
  [SYCL][NFC] Add class layout ABI tests for memory objects (intel#2559)
  [SYCL] Change adress space for global variables (intel#2534)
  [NFC][SYCL] Fix comment. (intel#2541)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexbatashev alexbatashev alexbatashev left review comments

@romanovvlad romanovvlad romanovvlad left review comments

@s-kanaev s-kanaev s-kanaev left review comments

@sergey-semenov sergey-semenov sergey-semenov approved these changes

@againull againull Awaiting requested review from againull

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cperkinsintel @romanovvlad @alexbatashev @sergey-semenov @s-kanaev @bader