Optimize memory handling #1963

PhilippTakacs · 2024-06-06T14:50:50Z

By using the memory snapshots with real examples we found some bottlenecks. Therefor I have implemented some optimizations.

We found that the flatview creation is quite expensive (around 1/4 of all runtime including emulation) when you have a lot of memory regions. By permanently creating new regions with CoW we have a lot of regions and rebuild for each cow the flatview again. I have added a update function does only change affected ranges.

Also we found that the find_ram_offset_last optimization doesn't work after a restore. To fix this the context now includes also the ramblock_freed bool.

This also includes two smaller optimizations: only clear the affected tlb on memory_cow and save the last ramblock of the ram_list

It's currently only a draft, because we need to do a few more tests to see if the optimization works as expected.

PhilippTakacs · 2024-07-03T09:00:56Z

I have additional added the flatview to the context. This way flatview doesn't need to be rebuild on context_restore.

I have tested and profiled this. It does in fact save us relevant time.

PhilippTakacs · 2024-07-19T12:08:42Z

@wtdcode have you overseen this PR? It would be nice to get some feedback.

wtdcode · 2024-07-19T16:37:36Z

@wtdcode have you overseen this PR? It would be nice to get some feedback.

Sorry for not leaving a comment. I had a quick review on it and didn't find any obvious problems. I planned to have further debugging to have a better understanding before commenting further.

wtdcode · 2024-09-21T14:07:22Z

I ported d010357 which generally allows unicorn to go fast qemu_st path (not going through store_helper again) to have better writing performance. This shall have side effects on your memory_cow which relies on store_helper.

I have twinkled a bit on the code to ensure that when snapshot is enabled, no TLB is marked clean. However, I'm not sure if that works for you, you may have a look.

Regarding this PR, I locally tested and think it is good to go if you don't have more to add.

PhilippTakacs · 2024-10-01T09:53:27Z

Thanks for the update. As far as I understand this patch allows faster writes, if the tlb cache entry exists. So for snapshots it might be enough to clear the tlb cache. This way the store_helper would be called again and everything works as expected. I'll test this.

think it is good to go if you don't have more to add

I would like to rebase this to the dev branch to remove the "fixup" commits. Also because I haven't looked at this for a few weeks, I would like to test this again with the current HEAD. I'll ping you when I'm finished testing.

PhilippTakacs · 2024-10-11T13:13:39Z

@wtdcode I have tested the code again and removed the fixup commits so for me this is ready to merge.

For the notdirty write I'm working on a optimization, but I'll open another PR for it.

wtdcode · 2024-10-11T13:15:42Z

@wtdcode I have tested the code again and removed the fixup commits so for me this is ready to merge.

For the notdirty write I'm working on a optimization, but I'll open another PR for it.

Will check today or tomorrow, thanks for your work!

wtdcode · 2024-10-16T08:28:48Z

uc.c

@@ -2418,6 +2431,11 @@ uc_err uc_context_restore(uc_engine *uc, uc_context *context)
            return ret;
        }
        uc_snapshot(uc);
+        uc->ram_list.freed = context->ramblock_freed;


Sorry I should have noticed this earlier. With this change (and actually the early snapshot_level change), with UC_CTL_CONTEXT_MEMORY, the context is no longer safe to keep as a file (it contains pointers) and be restored to another uc_engine instance.

It is super non-trivial to adapt the memory context to a value type and thus I suggest adding a few documents in the header (uc_context_save/restore, UC_CTL_CONTEXT_MEMORY).

Yes, we had this question already in the snapshots feature. I'll write some documentation about this.

qemu/exec.c

wtdcode · 2024-10-16T09:24:04Z

uc.c

+    if (context->fv) {
+        free(context->fv->ranges);
+    }
+    g_free(context->fv);


I think this shall move to the if (context->fv) branch.

g_free() and free() with a NULL is a noop so it's not necesary. I can move it if you prefer it this way better.

Yes, please do so because it looks more straightforward.

wtdcode · 2024-10-16T09:25:21Z

qemu/softmmu/memory.c

        }
    }
-    memory_region_transaction_commit(mr);
+  //  memory_region_transaction_commit(mr);


Is it safe to remove this commit?

Yes. This function is called in two cases.

unmap/free complete memory region (as part of destroy)
Then the parrent unmap will trigger the commit

restore the memory state
Then the restore also restores the old flatview so no rebuild is nesecary

I did a quick check, and it seems to make sense.

Could you also add these as comments here? So that I know the preconditions here if we are going to bump qemu version.

wtdcode · 2024-10-16T10:49:44Z

Okay, I finished the review, and it's safe to go now with minor fixes and a few document updates. Thanks again for your brilliant work.

Save the last element of the ram_list. This allows to faster find where to add new elements when they are not bigger then page size.

this keeps the optimization for find_ram_offset() intact after snapshot restore.

Building each flatview new when the memory has changed is quite expensive when many MemoryRegions are used. This is an issue when using snapshots.

this avoids rebuilding the flatview when restore a context.

Specialy stress that with UC_CTL_CONTEXT_MEMORY it is not possible to use the context with a different unicorn object.

PhilippTakacs · 2024-10-16T12:21:10Z

I have added the last fixes and rebased it to the dev branch.

The failed test doesn't look like it's a bug by my changes.

wtdcode · 2024-10-16T12:22:27Z

I have added the last fixes and rebased it to the dev branch.

The failed test doesn't look like it's a bug by my changes.

I will rerun and it seems Github Action bug.

wtdcode · 2024-10-16T13:51:18Z

Thanks!

PhilippTakacs force-pushed the optimize_ram_block_add branch from 3d204e4 to aa296ad Compare July 3, 2024 08:41

PhilippTakacs marked this pull request as ready for review July 3, 2024 09:00

PhilippTakacs force-pushed the optimize_ram_block_add branch from aa296ad to de2ae81 Compare October 11, 2024 11:21

wtdcode reviewed Oct 16, 2024

View reviewed changes

qemu/exec.c Show resolved Hide resolved

wtdcode reviewed Oct 16, 2024

View reviewed changes

PhilippTakacs added 9 commits October 16, 2024 13:44

optimize ram block handling

2a961e1

Save the last element of the ram_list. This allows to faster find where to add new elements when they are not bigger then page size.

save ram_list freed

e52dcf9

this keeps the optimization for find_ram_offset() intact after snapshot restore.

cow only clear the tlb of affected pages

08513a3

update flatview when possible

f43ed19

Building each flatview new when the memory has changed is quite expensive when many MemoryRegions are used. This is an issue when using snapshots.

update benchmark for new api

03a3fb5

save flatview in context

55231de

this avoids rebuilding the flatview when restore a context.

init context flatview with zero

2868e3e

address_space_dispatch_clear remove subpage with higher priority

73264ac

docutemnt the options for UC_CTL_CONTEXT_MODE

bd33da3

Specialy stress that with UC_CTL_CONTEXT_MEMORY it is not possible to use the context with a different unicorn object.

PhilippTakacs force-pushed the optimize_ram_block_add branch from de2ae81 to bd33da3 Compare October 16, 2024 11:51

wtdcode merged commit e8ca3cb into unicorn-engine:dev Oct 16, 2024
33 checks passed

wtdcode mentioned this pull request Oct 23, 2024

Question on saving and restoring memory snapshots #2043

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize memory handling #1963

Optimize memory handling #1963

PhilippTakacs commented Jun 6, 2024

PhilippTakacs commented Jul 3, 2024

PhilippTakacs commented Jul 19, 2024

wtdcode commented Jul 19, 2024

wtdcode commented Sep 21, 2024 •

edited

Loading

PhilippTakacs commented Oct 1, 2024

PhilippTakacs commented Oct 11, 2024

wtdcode commented Oct 11, 2024

wtdcode Oct 16, 2024 •

edited

Loading

PhilippTakacs Oct 16, 2024

wtdcode Oct 16, 2024

PhilippTakacs Oct 16, 2024

wtdcode Oct 16, 2024

wtdcode Oct 16, 2024

PhilippTakacs Oct 16, 2024

wtdcode Oct 16, 2024

wtdcode commented Oct 16, 2024

PhilippTakacs commented Oct 16, 2024

wtdcode commented Oct 16, 2024

wtdcode commented Oct 16, 2024

Optimize memory handling #1963

Optimize memory handling #1963

Conversation

PhilippTakacs commented Jun 6, 2024

PhilippTakacs commented Jul 3, 2024

PhilippTakacs commented Jul 19, 2024

wtdcode commented Jul 19, 2024

wtdcode commented Sep 21, 2024 • edited Loading

PhilippTakacs commented Oct 1, 2024

PhilippTakacs commented Oct 11, 2024

wtdcode commented Oct 11, 2024

wtdcode Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

PhilippTakacs Oct 16, 2024

Choose a reason for hiding this comment

wtdcode Oct 16, 2024

Choose a reason for hiding this comment

PhilippTakacs Oct 16, 2024

Choose a reason for hiding this comment

wtdcode Oct 16, 2024

Choose a reason for hiding this comment

wtdcode Oct 16, 2024

Choose a reason for hiding this comment

PhilippTakacs Oct 16, 2024

Choose a reason for hiding this comment

wtdcode Oct 16, 2024

Choose a reason for hiding this comment

wtdcode commented Oct 16, 2024

PhilippTakacs commented Oct 16, 2024

wtdcode commented Oct 16, 2024

wtdcode commented Oct 16, 2024

wtdcode commented Sep 21, 2024 •

edited

Loading

wtdcode Oct 16, 2024 •

edited

Loading