Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[patch] decrease resident memory usage. #256

Closed
ramosian-glider opened this issue Aug 31, 2015 · 9 comments

Comments

Projects
None yet
2 participants
@ramosian-glider
Copy link
Member

commented Aug 31, 2015

Originally reported on Google Code with ID 256

Hi,

Here is a patch that helps decreasing ASan's memory usage. On my use case, this decrease
memory consumption by 20% with no noticeable performance impact (in fact, it seems
the resulting process is marginally faster).

The idea here is to clear memory using a fixed mmap instead of memset since both will
result in zeroed memory but mmap memory is not resident before someone writes something
in it. This also could result in faster code for large clear since memset scans the
memory and mmap only change the virtual address map, however while memset runs in userland,
mmap implies performing a syscall that will take a lock to modify a shared (between
threads) resource.

Reported by florent.bruneau@intersec.com on 2013-12-29 11:28:16


- _Attachment: [0001-asan-reduce-resident-memory-consumption.patch](https://storage.googleapis.com/google-code-attachments/address-sanitizer/issue-256/comment-0/0001-asan-reduce-resident-memory-consumption.patch)_
@ramosian-glider

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2015

>> On my use case
What is your case? 
I suspect it is doing huge allocations and not using most of the allocated memory.
Not the very common case. 

Please, provide a benchmark.
Very likely this patch will slowdown the common case of relatively small allocations.

Reported by konstantin.s.serebryany on 2014-01-09 04:57:24

  • Labels added: Type-Enhancement
  • Labels removed: Type-Defect
@ramosian-glider

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2015

My use case is the binary that contains all the unit tests for the base library, it
contains a lot of small and some huge allocations. The huge allocations (a few mmap
of 256MiB) are quite rare but enough to have an impact on the amount of resident memory
consumed by Asan's shadow memory.

I ran the attached benchmark (a binary tree with resizable arrays in the nodes) on
a Linux desktop (Linux 3.8). This gave the following result (best occurrence given
for both):
* without my patch:
        User time (seconds): 77.39
        System time (seconds): 0.26
        Maximum resident set size (kbytes): 500176

* with my patch:
        User time (seconds): 77.42
        System time (seconds): 0.30
        Maximum resident set size (kbytes): 499168

AFAICT, the difference in term of performance is well under the measure error threshold,
so I tend to conclude that the patch has no noticeable impact on performance (or at
most a very tiny slowdown).

In both cases, the program was compiles with clang 3.4 with the following command line:

 clang -O2 -g -fsanitize=address -o blah blah.c

Let me know if you have a better idea for a meaningful benchmark.

Reported by florent.bruneau.2003@polytechnique.org on 2014-01-09 08:38:32


- _Attachment: [blah.c](https://storage.googleapis.com/google-code-attachments/address-sanitizer/issue-256/comment-2/blah.c)_
@ramosian-glider

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2015

The difference in resident set size is 1Mb, or 0.2% of the overall size.
This is not a convincing gain. 

Also, a benchmarks should
  - have less unrelated logic (no trees, nodes, etc. Just malloc/free)
  - have multiple threads, all malloc-intensive
  - have small allocations as well as large allocations
  - have most of the malloc-ed memory actually used

Note also, that once we free the large malloc-ed block we call FlushUnneededASanShadowMemory
(madvise((void*)addr, size, MADV_DONTNEED);)
so if the large blocks are actually freed, the shadow should be fully freed as well.

Reported by konstantin.s.serebryany on 2014-01-09 08:54:02

@ramosian-glider

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2015

I didn't expect the resident set size to differ in this bench since my patch only affects
large allocations, not small ones.

AFAICT, the FlushUneededShadowMemory() is called for unmaps only. My patch catches
every single unpoising.

Reported by florent.bruneau@intersec.com on 2014-01-09 09:19:44

@ramosian-glider

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2015

>> My patch catches every single unpoising.
Exactly. mmaps are expensive. 
Unless I see a proof that the proposed approach does not slowdown the common use-case,
I am not willing to accept this patch. 
Closing now, please reopen once you have convincing benchmarks. 

Reported by konstantin.s.serebryany on 2014-01-29 13:16:08

  • Status changed: WontFix
@ramosian-glider

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2015

>> My patch catches every single unpoising.
> Exactly. mmaps are expensive.

memsetting 4k of memory is also expensive.

> Unless I see a proof that the proposed approach does not slowdown the common use-case,

I'll provide the requested benchmark as soon as possible (but I have professional constraint
and cannot spend too much time on it for now). That said, you should seriously consider
adding a reference benchmark in the repository if you expect contributions, because
you can hardly ask for a benchmark to every single person who propose a patch.

Reported by florent.bruneau@intersec.com on 2014-02-01 16:31:11

@ramosian-glider

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2015

I've applied something based on your patch in r201400, but we start using mmap instead
of memset at a larger region size (64K by default).

Reported by eugenis@google.com on 2014-02-14 11:48:14

  • Status changed: Fixed
@ramosian-glider

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2015

thx

Reported by florent.bruneau@intersec.com on 2014-02-14 15:58:13

@ramosian-glider

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2015

Adding Project:AddressSanitizer as part of GitHub migration.

Reported by ramosian.glider on 2015-07-30 09:13:43

  • Labels added: ProjectAddressSanitizer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.