Remap pointers in `decompression_context` and `database_context`? about acl HOT 4 CLOSED

Hello @EAirPeter,
Your analysis is correct. The compressed tracks and database data is a binary blob that can easily be relocated with memcpy.

For the decompression context, it may be easier to re-create it on demand on the stack whenever you need to decompress. That is what the Unreal Engine plugin does and what most engines do. Re-using the context will only save you its construction/initialization cost which is very cheap. If you wish to re-use it after relocation you could simply call initialize again with your new compressed tracks/database pointers/references. You'll need to seek again as well before decompression but you probably do that right now anyway. The decompression context itself can also be relocated with memcpy if need be.

For the database context, things are a bit different. You can't initialize twice, but you can call reset before you initialize it again. The database context allocates memory internally and as such we need to free it. You'll likely want to reset it before the memory is reclaimed in your scheme to prevent use after free. How you handle the database streamed data is up to you within the streamer object. You could make the stream in/out option a no-op if the data is already present or you could opt to truly stream in/out altogether.

Getting rid of some of these pointers is something I'd like to do. Storing offsets is more compact and just as fast on x64 (single instruction). That's doable for the decompression context with the exception of the pointers to the compressed tracks/database. Those must remain as proper pointers as it is illegal in C++ to compute an offset from two pointers not part of the same aggregate structure/blob. It may or may not work depending on the platform. On x64, memory is unified and as such it might work but that is not always the case.

For the database context, we could similarly store offsets for most things but two pointers will remain: the compressed database and the base of the dynamic allocation for metadata tracking. Possibly, we could add a function to allocate both the database context AND the metadata buffer together as one blob. This would allow us to remove the pointer to the metadata since we could use this and an offset. However, this would force everyone to use that API which might not be desirable. I think this might be something we can handle through the database_settings at compile time. The context could store a union of a pointer and offset along with a function that returns the pointer either computed from this with the offset or returns the pointer. This would be a good middle ground I think.

For the database context, there is also the question of how we migrate the allocated metadata. If we had everything in one blob, then relocation would work well. We would simply need to fixup the compressed database pointer. But what about the information regarding what is streamed in/out? If you don't stream things out through the API to update the metadata before your relocation, the context will think things are still streamed in after relocation and the onus will be on you to make sure that is the case.

To summarize, I would like to propose the following changes to better address your use case:

• Introduce a reset function on the decompression_context to mirror the behavior in the database_context even if it isn't strictly required. This will make the API symmetrical and consistent.
• Introduce a relocate function on both context objects to fixup the required pointers.
• Convert as many things as possible into offsets to reduce the number of pointer fixup required.
• Expose a new API to allocate the database_context together with its metadata so that both can be relocated together in one blob and so that we may use offsets there as well.

How does that sound?

from acl.

After digging more into code, I found that those contexts actually store only the pointers to the tracks, database and bulk data themselves, not to any members of them. It turns out that I can achieve what I want simply by updaing the tracks/db pointer and make sure this does not happen between seek and decompress_track|tracks.

from acl.

Sounds great. Currently, I achieve my goal through a similar relocate function and it works smoothly. Thank you so much, looking forward to new versions.

from acl.

I merged a fix for this along the lines of what we discussed.
A reset function has been added.
I renamed is_dirty to is_bound_to to clarify usage and meaning.
I added a relocated function (not relocate to clarify that it does not relocate anything) that does the pointer fixup.
After review, no further pointers were fixed to offsets. In the decompression context, aside from the compressed tracks and database pointers, the others are fixed during the call to seek. I simply ensured that seek must be called following relocation which seems entirely reasonable to me.
I have not added any functionality to relocate the context objects themselves. If this is needed, I'll tackle it later.

from acl.