Perf optimization for writing one byte in memory

[catenacyber]

Tested on ais3_crackme example

Function write called 19164 times goes down from 2.076 seconds cumulated to 0.914 seconds by using this new write1 function (on 42 seconds total time)

This code may be less readable though...

---

This change is 

[ehennenfent]

It looks like most of the Travis failures are TypeError: 'int' object is not subscriptable on line 299 of memory.py. Can you add a type check for that, or does it negate the performance improvement?

[catenacyber]

Thank you @ehennenfent

I just changed the order of type checks.
It looks like the line self._data[index] = Operators.ORD(value) in AnonMap.__setitem__ was not reachable because we were first trying to access value[0]
Now we only access value[0] if we are dealing with a slice

[ehennenfent]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @catenacyber)

---

manticore/native/memory.py, line 966 at r3 (raw file):

        if m is None:
            raise InvalidMemoryAccess(addr, "w")
        if not force and not m.access_ok("w"):

Checking the memory permissions on each byte is going to be slow. We'll approve this for now, but in the future we should try to make this faster by checking the permissions for the first and last byte of each write and skipping over per-byte checks if the beginning and end are both valid (and in the same map)

[catenacyber]

Checking the memory permissions on each byte is going to be slow.

But we have only one byte as that is the point of this PR.
Or do you mean that this loop (one level up the stack) should have been optimized instead ?

manticore/manticore/native/memory.py

Line 1180 in 81f8f97

for offset in range(size):

[ehennenfent]

do you mean that this loop (one level up the stack) should have been optimized instead?

Not necessarily "instead", but yes, we should modify the loop such that it's not necessary to check the memory permissions for every single byte. We can save that for a different PR.

[catenacyber]

Not necessarily "instead", but yes, we should modify the loop such that it's not necessary to check the memory permissions for every single byte. We can save that for a different PR.

I guess I will do the upper loop modification first and then see if this PR is always relevant

[catenacyber]

With the upper loop modification, I get 0.6 seconds cumulated for ais3_crackme with command python3 -m cProfile -o cprofall.txt win.py

19167    0.100    0.000    0.576    0.000 memory.py:1160(write)

With only the upper loop change (ie not using write1), I got 0.7 seconds and without any changes 2.1 seconds

[disconnect3d]

I think that the CI fails with a timeout because we removed this. It might be that it ends up in an infinite loop.

Also, the test_mix_of_concrete_and_symbolic test fails.

In this test we:

1. Create memory with 0, 2, 4, 6 offsets/indexes/memory cells as concrete values and 1, 3, 5, 7 as symbolic.
2. Assert if those concretes/symbolic addresses are really concrete/symbolic.
3. Swap indexes of concrete/symbolic memory cells and reinitialise memory bytes to concrete/symbolic values.
4. We reassert those. Here, the test fails.

This is because we don't delete the old symbolic mappings, so in mem._symbols we have old 1, 3, 5, 7 symbolic cells and 0, 2, 4, 6 new symbolic cells. The 1, 3, 5, 7 should not be there.

[disconnect3d]

I am going to try fixing that by readding the del ... and see if it works.

[catenacyber]

Indeed

[dguido]

oops, I think you may have accidentally automatically closed this because of the text in #1506

[disconnect3d]

I spend some time on this code/issue, analyzed it and found out that yes, indeed we need this or similar improvement, but the code present here is not complete and a bit overcomplex imho.

I am closing this issue and I am going to propose another solution along with other improvements.

By the way, the benchmark used here - manticore-examples/test_ais3_crackme - is not a good benchmark to show the issue as it does not involve a lot of memory writes.

A better one is here:

import time
from manticore.core.smtlib import ConstraintSet
from manticore.native.memory import SMemory32

def test_write_concrete():
    t1 = time.time()
    mem = SMemory32(ConstraintSet())
    buf = mem.mmap(None, 0x1000, "rw")

    for i in range(1000):
        mem[buf:buf+0x1000] = "a" * 0x1000

    t2 = time.time()
    print(f"test_write_concrete: {t2-t1} seconds")

test_write_concrete()

Here are my results:

• current master or cdda4ee: test_write_concrete: 54.29297089576721 seconds
• this PR: test_write_concrete: 2.8424699306488037 seconds
• my solution, that does not introduce write1 and only chunks the writes: test_write_concrete: 3.17004132270813 seconds

Just to be clear, the current PR has the following issues:

• it does not clear symbolic chunk writes when writing concrete values
• it does not support writes with partial symbolic and concrete values which works on master

[catenacyber]

Bravo for the rework, glad I could help.

By the way, the benchmark used here - manticore-examples/test_ais3_crackme - is not a good benchmark to show the issue as it does not involve a lot of memory writes.

Interesting.
I am looking for good benchmarks in a general way.
test_ais3_crackme is good as it is a real use case
We will not find worst case scenario in it but we can improve the performance in the general case.

my solution, that does not introduce write1

I agree that write1 is not so good to introduce
But Eric told me to keep it here #1465 (comment)

it does not support writes with partial symbolic and concrete values which works on master

How so ?

[disconnect3d]

Bravo for the rework, glad I could help.

Thanks :).

test_ais3_crackme is good as it is a real use case

Yeah, it is good as a real use case but it might be hard (and slow) to measure the right impact/improvement in regards to the given functionality when using it.

I agree that write1 is not so good to introduce
But Eric told me to keep it here #1465 (comment)

I guess we can play with it in the future, but for now its speed-up is probably negligible.

it does not support writes with partial symbolic and concrete values which works on master

How so ?

Assignments like mem[0:2] = (cs.new_bitvec(8), b'a') stopped working. We haven't tests for it before, but I've added such in #1509.