Skip to content
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.

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

Remove Usage of std::set in ResidencySet #547

Merged
merged 1 commit into from
Aug 2, 2022
Merged

Conversation

bjjones
Copy link
Contributor

@bjjones bjjones commented Jul 29, 2022

Removes usage of std::set in ResidencySet and replaces it with a std::vector.
This does allow duplicate heaps to be inserted into the ResidencySet,
however insertion will be O(1) instead of O(log(n)) and instead we can
easily identify duplicate heaps by checking their last used fence value
when during iteration.

@github-actions github-actions bot added D3D12 DirectX 12 Backend Change Test Changes in tests. labels Jul 29, 2022
@bjjones
Copy link
Contributor Author

bjjones commented Jul 29, 2022

I know there are some rough aspects to this PR, but just let me know what you think of the idea.

@bbernhar
Copy link
Contributor

SGTM. Nice idea.

@bbernhar
Copy link
Contributor

Can you rebase and push?

@bjjones
Copy link
Contributor Author

bjjones commented Jul 29, 2022

Done. Still failing.

@bbernhar
Copy link
Contributor

Alright, please rebase again.

@bbernhar
Copy link
Contributor

Cool! That did it, thanks.

I like your approach, let me know when I can review.

@bjjones
Copy link
Contributor Author

bjjones commented Aug 1, 2022

You can review. Mostly just renaming things.

Copy link
Contributor

@bbernhar bbernhar left a comment

Choose a reason for hiding this comment

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

LGTM but ResidencySet is an exported interface (public), so you'll also need to depreciate it or the integrations will fail.

src/gpgmm/d3d12/ResidencyListD3D12.h Outdated Show resolved Hide resolved
src/gpgmm/d3d12/ResidencyListD3D12.h Outdated Show resolved Hide resolved
@bbernhar bbernhar added the breaking-change Breaking Change label Aug 1, 2022
@github-actions github-actions bot added the Build Changes to build system. label Aug 1, 2022
@bjjones
Copy link
Contributor Author

bjjones commented Aug 1, 2022

Re-added ResidencySet + ExecuteCommandLists ResidencySet wrapper with /deprecated tag. Please lmk if this is what you meant.

@bjjones bjjones force-pushed the removeset branch 2 times, most recently from 21c653e to 38514ca Compare August 1, 2022 21:20
@bbernhar
Copy link
Contributor

bbernhar commented Aug 1, 2022

Looks good to me, thanks for fixing up the docs.

FYI, the Dawn integration isn't up-streamed (obviously), so if you want to fully test you can git apply .github\workflows\.patches\dawn.diff, change it, and create/upload a new diff. WebNN integration is up-streamed, so the only way to update it is to merge here then roll GPGMM & submit PR to WebNN.

@bjjones
Copy link
Contributor Author

bjjones commented Aug 1, 2022

Is D3D12ResidencyManagerTests.CreateResidencyManagerNoLeak known to be flaky? It failed but it does not appear I've made any changes that would cause it to fail.

@bbernhar
Copy link
Contributor

bbernhar commented Aug 1, 2022

Not sure we can call it flaky yet - only 1 run failure on that patchset.

Re-running it again,
https://github.com/intel/GPGMM/actions/runs/2778014818

@bjjones
Copy link
Contributor Author

bjjones commented Aug 1, 2022

It flaked on GN/MSVC Debug initially, then passed:

https://github.com/intel/GPGMM/actions/runs/2778014810

@bbernhar
Copy link
Contributor

bbernhar commented Aug 1, 2022

Good catch, filed #553

Removes usage of std::set in ResidencySet and replaces it with a vector.
This does allow duplicat heaps to be inserted into the ResidencySet,
however insertion will be O(1) instead of O(log(n)) and instead we can
easily identify duplicate heaps by checking their last used fence value
when when they are iterated.
@bjjones bjjones merged commit a4b48e7 into intel:main Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Breaking Change Build Changes to build system. D3D12 DirectX 12 Backend Change Test Changes in tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants