Page MenuHomePhabricator

[esan|wset] Iterate all memory to compute the total working set

Authored by bruening on May 24 2016, 10:20 AM.



Adds iteration of all application memory in an efficient manner using
shadow faults. Shadow memory starts out inaccessible and we mark it
writable one page at a time on each fault when the instrumentation touches
it. This allows iteration over just the mapped shadow memory, saving
significant time.

Adds a process-end iteration and pretty-printing of the final result.

Adds a new test and updates the existing tests.

Diff Detail


Event Timeline

bruening updated this revision to Diff 58263.May 24 2016, 10:20 AM
bruening retitled this revision from to [esan|wset] Iterate all memory to compute the total working set.
bruening updated this object.
bruening added a reviewer: aizatsky.
bruening added subscribers: llvm-commits, eugenis, kcc and 2 others.
aizatsky added inline comments.May 24 2016, 2:36 PM
79 ↗(On Diff #58263)

This function changes shadow value. Should be at least reflected in the name.

Plus "processShadowRegion" is not very telling.

105 ↗(On Diff #58263)


E.g. "computeWorkingSizeAndClearRecent".

130 ↗(On Diff #58263)

symbolic constant for 64 please.

132 ↗(On Diff #58263)


69 ↗(On Diff #58263)

Should this be working_set_linux.cpp then?

filcab added a subscriber: filcab.May 25 2016, 8:21 AM
filcab added inline comments.
85 ↗(On Diff #58263)

RoundDown? We'll be looking at some shadow bytes before ShadowStart. Won't that be a problem?

89 ↗(On Diff #58263)


90 ↗(On Diff #58263)

ByteValue has only one bit set. No need to test for equality.

129 ↗(On Diff #58263)

We don't really care, since we only do this at finalize time (and we're printing stuff anyway), no?

131 ↗(On Diff #58263)

No sense, again. Just make a global kDefaultCacheLineSize, and document next to it that we only support that size.

69 ↗(On Diff #58263)

or at least the _posix variant.

18 ↗(On Diff #58263)

What do you mean?

bruening updated this revision to Diff 58506.May 25 2016, 2:08 PM
bruening marked 9 inline comments as done.

Address reviewer requests.

85 ↗(On Diff #58263)

No, this routine may be used for handling smaller regions in the future (e.g., just the heap) where it is convenient to not require precision or alignment from the caller.

129 ↗(On Diff #58263)

There are linker errors for missing symbols like __cxa_guard_acquire which would require adding libraries these sanitizer libs try to avoid.

69 ↗(On Diff #58263)

POSIX specifies that si_addr be filled in for SIGSEGV. It's older Linux that fails to do so (we're talking really old: 10+ years IIRC).

filcab accepted this revision.May 26 2016, 8:38 AM
filcab added a reviewer: filcab.


35 ↗(On Diff #58506)

Should this be in the header?

82 ↗(On Diff #58506)

We should probably mention it might count and clear a few bytes more than [ShadowStart,ShadowEnd)

This revision is now accepted and ready to land.May 26 2016, 8:38 AM
bruening added inline comments.May 26 2016, 10:04 AM
35 ↗(On Diff #58506)

We can move it to the header once another file needs it.

82 ↗(On Diff #58506)

That is what the word-align comment implies.

vitalybuka added inline comments.May 26 2016, 10:46 AM
87 ↗(On Diff #58506)

Is this the same as?
u32 WordValue = 0x01010101 << BitIdx;

eugenis requested changes to this revision.May 26 2016, 10:57 AM
eugenis added a reviewer: eugenis.

This revision depends on r270650, which broke the bots. Please fix that first.

This revision now requires changes to proceed.May 26 2016, 10:57 AM

The bot is now green.

eugenis accepted this revision.May 27 2016, 11:04 AM
eugenis edited edge metadata.
This revision is now accepted and ready to land.May 27 2016, 11:04 AM
This revision was automatically updated to reflect the committed changes.