Add log connectivity parameter to fossilize-list - #284
Conversation
This comment was marked as spam.
This comment was marked as spam.
| bool replayer_create_info_fill(ResourceTag selected_tag, StateReplayer& replayer, ListReplayer& list_replayer, const std::unique_ptr<DatabaseInterface>& input_db) | ||
| { | ||
| vector<uint8_t> state_db; | ||
| for (auto tag : playback_order) |
There was a problem hiding this comment.
This looks like O(n^2) explosion, which won't fly for larger archives. The outer loop loops over all hashes, and this also loops over all hashes. Any particular reason why it has to be this way?
There was a problem hiding this comment.
This is done because the replayer needs to know in advance about some Vulcan objects for the selected object, in order to write them to create info. However, I can go through all the database hashes once and collect all the information in an array of structures in the ListReplayer itself. This should be cheaper than what is currently happening.
There was a problem hiding this comment.
The design here feels like it would be far simpler if you just replay the blobs with a trivial impl of object = hash, then look at the resulting create infos, and print the relevant objects embedded. Like for pipelines, loop over the stages, and print the module, as well as renderPass/layout, etc.
This implementation seems overly complicated and slow and likely needs a rewrite.
| unordered_set<Hash> saved_raytracing_pipelines; | ||
| }; | ||
|
|
||
| map<Hash, SavedHashes> saved_hashes_map; |
There was a problem hiding this comment.
Why is this an ordered map?
There was a problem hiding this comment.
Also, you need different hash maps per resource type. Hashes from difference resource types are allowed to collide.
| { | ||
| struct SavedHashes | ||
| { | ||
| unordered_set<Hash> saved_samplers; |
There was a problem hiding this comment.
This feels like it should just be a vector of pair<tag, hash>. There's no reason why every CLI invocation should give unstable results in stdout due to iterating over these hashmaps. Hashmap is also overkill for this scenario.
|
|
||
| bool replayer_create_info_fill(ResourceTag selected_tag, StateReplayer& replayer, ListReplayer& list_replayer, const std::unique_ptr<DatabaseInterface>& input_db) | ||
| { | ||
| // fill Vulcan object data in replayer |
| using namespace Fossilize; | ||
| using namespace std; | ||
|
|
||
| #define PRINT_SAVED_BUFFER(SavedBufferName, VkObjectName, Tag)\ |
There was a problem hiding this comment.
I don't think you need a macro just to print out an array.
| if (log_connectivity) | ||
| { | ||
| size_t state_db_size; | ||
| if (!input_db->read_entry(tag, hash, &state_db_size, nullptr, 0)) |
There was a problem hiding this comment.
I'm a little confused why you're reading the blob here. It's not used for anything?
There was a problem hiding this comment.
forgot to clean up after previous commits.
| { | ||
| for (auto par : saved_hashes_map->second) | ||
| { | ||
| printf(tag_names[par.first - 1 >= tag_names_size ? tag_names_size - 1 : par.first - 1]); |
There was a problem hiding this comment.
Why the minus 1?
There was a problem hiding this comment.
Last index of tag_names
There was a problem hiding this comment.
I mean, why is this not just tag_names[par.first] ? There should be no OOB to worry about and the table could be 1:1 with resource tag types.
There was a problem hiding this comment.
The reason is that RESOURCE_RAYTRACING_PIPELINE = 9
There was a problem hiding this comment.
Can't the table just accomodate all possibilities, padding with empty strings as necessary? There's no need to complicate this more than it needs to be.
Co-authored-by: Hans-Kristian Arntzen <post@arntzen-software.no>
Co-authored-by: Hans-Kristian Arntzen <post@arntzen-software.no>
Co-authored-by: Hans-Kristian Arntzen <post@arntzen-software.no>
Co-authored-by: Hans-Kristian Arntzen <post@arntzen-software.no>
Co-authored-by: Hans-Kristian Arntzen <post@arntzen-software.no>
| if (selected_tag != RESOURCE_COMPUTE_PIPELINE) | ||
| return true; | ||
|
|
||
| auto saved_hash_iter = saved_hashes_map.insert({ hash, saved_hashes_type() }).first; |
There was a problem hiding this comment.
It's much easier to read the plain:
auto &dependencies = saved_hashes_map[hash];
dependencies.push_back(...);
No need to use iterators here.
| #include "layer/utils.hpp" | ||
| #include <memory> | ||
| #include <vector> | ||
| #include <unordered_set> |
|
A general tip: When applying code suggestions on github you can batch them up and commit them in one go instead of generating 10 commits for each individual suggestion. (I'll squash it anyway, but reduces clutter a bit). |
…o-fossilize-list2' into Add-log-connectivity-parameter-to-fossilize-list2
| if (selected_tag != RESOURCE_PIPELINE_LAYOUT) | ||
| return true; | ||
|
|
||
| saved_hashes_map.insert({ hash, saved_hashes_type() }); |
There was a problem hiding this comment.
You don't need to insert. It's implied by saved_hashes_map[hash].
There was a problem hiding this comment.
Oh, didn't know about it, thanks.
|
I removed the constants in the arguments of some functions (input_db), as the methods called by these constants are not constant at all. |
2 participants
This PR adds the
--connectivityparameter tofossilize-list. Connectivity in this case refers to the hashes of objects referenced by the hash of the selected tag (via the--tagparameter), or in other words, the hashes of objects associated with that hash.Example for Graphics Pipeline:
