Skip to content

[RFC] layers: Split parts of core_validation out#848

Merged
mark-lunarg merged 2 commits intoKhronosGroup:masterfrom
JasperNV:jstpierre/core-validation-split
Apr 11, 2019
Merged

[RFC] layers: Split parts of core_validation out#848
mark-lunarg merged 2 commits intoKhronosGroup:masterfrom
JasperNV:jstpierre/core-validation-split

Conversation

@JasperNV
Copy link
Copy Markdown
Contributor

@JasperNV JasperNV commented Apr 8, 2019

This is a bikeshed proposal. Currently, core_validation.cpp is a single
file that is over 12,000 lines long. It takes a long time to compile,
can be exhausting to navigate, and some tools like IntelliSense can have
issues with its large size. Splitting it into multiple files isn't a
panacea for the above issues, but a small step towards a larger goal.

Here, I do drawdispatch as a simple example to show the idea itself and
build approval. It was small and self-contained and relatively
conflict-free. If this is well-received, I will do the work myself to
help split core_validation.cpp into multiple more files.

@mark-lunarg
Copy link
Copy Markdown
Contributor

This is great, and it's been on the list forever (some attempts were made -- buffer_validation, descriptor_sets, etc.,). Once the android build is sorted, this seems like a great direction. Can you share what kind of granularity you are looking toward -- for example, should drawdispatch eventually contain more CV stuff, or would there be LOTS of smaller files? Maybe a super-rough outline of what the next few files would be and what they might contain.

Also, I think it's fine if we just call it cv_drawdispatch.cpp instead of core_validation_drawdispatch.cpp, maybe @tobine could weigh in here. I can also go back and rename the other CV files to match.

Thanks @JasperNV...

@JasperNV
Copy link
Copy Markdown
Contributor Author

JasperNV commented Apr 9, 2019

I don't have any specific goals for granularity -- I figured splitting up the current core_validation.cpp into around 7-8 files seemed about right given its current size. I was imagining one file per important API concept, grouped as needed -- core_validation_buffer.cpp, core_validation_imagesampler.cpp, core_validation_descriptorset.cpp, core_validation_pipeline.cpp. Ultimately, the goal is to be pragmatic more than anything, so if the files are too granular, or not granular enough, we can change and adapt it.

I'll dig into the Android build issue.

@JasperNV JasperNV force-pushed the jstpierre/core-validation-split branch from e7670ab to 951c4e1 Compare April 9, 2019 17:01
This is a bikeshed proposal. Currently, core_validation.cpp is a single
file that is over 12,000 lines long. It takes a long time to compile,
can be exhausting to navigate, and some tools like IntelliSense can have
issues with its large size. Splitting it into multiple files isn't a
panacea for the above issues, but a small step towards a larger goal.

Here, I do drawdispatch as a simple example to show the idea itself and
build approval. It was small and self-contained and relatively
conflict-free. If this is well-received, I will do the work myself to
help split core_validation.cpp into multiple more files.
@JasperNV JasperNV force-pushed the jstpierre/core-validation-split branch from 951c4e1 to 7eef5eb Compare April 9, 2019 18:39
@JasperNV
Copy link
Copy Markdown
Contributor Author

JasperNV commented Apr 9, 2019

Android build issues have been fixed

@JasperNV
Copy link
Copy Markdown
Contributor Author

Quick ping on this?

@mark-lunarg
Copy link
Copy Markdown
Contributor

This is fine by me, but I'd like the filename changed from core_validation_drawdispatch to just drawdispatch. Is there more stuff planned to go into this file?

@JasperNV
Copy link
Copy Markdown
Contributor Author

JasperNV commented Apr 11, 2019

Nope. The motivation behind the name was to make it clear this was part of the core validation layer, not the other layers, since there's also the thread-safety, object-tracker, stateless-validation layers, etc. I am not married to the name, so I'll change it.

@JasperNV
Copy link
Copy Markdown
Contributor Author

Renamed to drawdispatch.cpp

@mark-lunarg
Copy link
Copy Markdown
Contributor

For a little extra context, in a month or two there will no longer a core_validation layer. There is a CoreChecks validation object, so that might be better, or maybe just cc_ or something instead. I really like the idea, and it's cool if we come up with a scheme that we can use for all of them, but at that time we should rename the other files as well to get the same advantage.

@JasperNV
Copy link
Copy Markdown
Contributor Author

Ah, that makes sense then. Any public documentation I can see about the proposed new structure for the layers?

@mark-lunarg
Copy link
Copy Markdown
Contributor

The white-paper is in final review now and should be available in next week's SDK. Most of it you can already see in the source -- the old layers have all been converted to ValidationObjects and get combined into the khronos_validation layer. Going forward, there'll be one validation layer, and you can flip parts of it on and off at CreateInstance-time. So this is really bending the idea of what a 'validation layers' means in this repo, and it seems like more terminology will be changing in the short-term as well. And I love the idea of making it obvious what the source relates to just from the filename

@mark-lunarg
Copy link
Copy Markdown
Contributor

Passed internal CI, good to go!

@mark-lunarg mark-lunarg merged commit 59a2964 into KhronosGroup:master Apr 11, 2019
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