bd: Add ProfilingBlockDevice for measuring higher-level applications

[geky]

This utility block device just has internal counters for keeping track of the number of bytes read/programmed/erased. Required for filesystem benchmarking.

[pan-]

I believe counter should only be incremented if the operation they monitor succeed.

I also wonder if it is necessary to make reset, get_read_count, get_write_count and get_erase_count virtual.

[pan-]

Increment of read count should be made only if the read operation was a success.

[pan-]

Increment of the program count should be made only if the program operation was a success.

[pan-]

Increment of the erase count should be made only if the erase operation was a success.

[geky]

I believe counter should only be incremented if the operation they monitor succeed.

Ah good point! I'll update

I also wonder if it is necessary to make reset, get_read_count, get_write_count and get_erase_count virtual.

I mean we already have a vtable ¯_(ツ)_/¯. I'll go ahead and make them non-virtual, I don't really have a preference.

[AnotherButler]

@geky Thanks for the PR.

Please change the PR subject line to "Add ProfilingBlockDevice to measure high-level applications" for clarity and brevity because we use our merged PR titles in our release notes.

[0xc0170]

Add ProfilingBlockDevice to measure high-level applications

What is difference to the current one - to measure vs for measuring plus the prefix? I like there the prefix bd: (the module that this applies to).

[pan-]

I mean we already have a vtable ¯_(ツ)_/¯. I'll go ahead and make them non-virtual, I don't really have a preference.

Those functions are getter to data member, I cannot think of any use case where overriding is necessary or even wanted.

But if a developer find a use case and derive the class, it would have to understand the relationship (and state modification) read, program, erase, _read_count, _program_count and _erase_count which can be confusing. It also makes the data member part of the public interface because they can be accessed in the derived classes.

My advice would be to makes data member private and remove the virtual specifier out of the reset, get_read_count, get_program_count and get_erase_count. If it is needed latter then open up the class for derivation.

[AnotherButler]

@0xc0170 We recommend our contributors follow Chris Beam’s seven rules of great commit messages. The current subject line breaks two of those rules [length and capitalization of the first letter]. Because "ProfilingBlockDevice" has "BlockDevice" in it, the prefix isn't necessary. It's not a big deal, but it's nice to have consistency in the release notes.

[geky]

@pan-, isn't that where calling the superclass's member functions by name becomes useful? (ie ProfilingBlockDevice::get_erase_count()).

Here's an interesting counter example, consider TimingProfilingBlockDevice, a block device that also measure the time operations take. We could extend the ProfilingBlockDevice to make use of it's existing function and would probably want to reuse reset:

class TimingProfilingBlockDevice : public ProfilingBlockDevice {
    // blablablabla

    virtual timestamp_t get_runtime() const {
        return _time;
    }

    virtual reset() {
        _time = 0; // reset our timing
        ProfilingBlockDevice::reset(); // also reset parent
    }
}

Though maybe this is only a concern for reset...

@AnotherButler, actually, reading through Chris Beam's rules, he doesn't really cover repositories with multiple modules like mbed OS. In the comments this same question came up, and it seems like the consensus is to use "module: Some title":
https://chris.beams.io/posts/git-commit/#comment-3077126915

Keeping a consistent module name is very useful for greping for related commits. (We don't have an enforced policy so it's not thaaat useful, but would be nice to have ;) )

[jupe]

maybe there could be also counter for failure cases where e.g. programming fails..

[pan-]

@geky @jupe Even if the class is extended to provide new kind of profiling features the data member _read_count, program_count and _erase_count have to stay private and there is no justification for overriding the getter functions.

The design of the TimingProfilingBlockDevice as presented can be improved, there is no need to have a relationship as strong as inheritance with the ProfilingBlockDevice.

Ideally a DataProfilingBlockDevice would track data read, program and erased, a TimingProfilingBlockDevice would instrument time taken by operations, a FaillureProfilingBlockDevice would instrument faillures, etc ...

It would also be possible to compose these all profiling utility types together in a non intrusive manner.

One solution would be to use a decorator. Similar to what has been done with the ProfilingBlockDevice class and a composite to have multiple profilers at once. However, it might not be easy to get access to the profilers data.

Another solution would be to decouple the instrumentation from the ProfileBlockdevice class by creating an InstrumentedBlockDevice class and delegate all the instrumentation task to a BlockDeviceInstrument:

struct BlockDeviceInstrument {
	virtual ~BlockDeviceInstrument() { }
	virtual void before_read(void *buffer, bd_addr_t addr, bd_size_t size) { }
	virtual void after_read(int result, void *buffer, bd_addr_t addr, bd_size_t size) { }
	virtual void before_program(const void *buffer, bd_addr_t addr, bd_size_t size) { }
	virtual void after_program(int result, const void *buffer, bd_addr_t addr, bd_size_t size) { }
	virtual void before_erase(bd_addr_t addr, bd_size_t size) { }
	virtual void after_erase(int result, bd_addr_t addr, bd_size_t size) { }
	virtual void reset() { }
};

class InstrumentedBlockDevice : public BlockDevice {
public:
	InstrumentedBlockDevice(BlockDevice* device, BlockDeviceInstrument& instrument) :
		_block_device(device), _instrument(instrument) { }

	virtual int read(void *buffer, bd_addr_t addr, bd_size_t size) {
		_instrument.before_read(buffer, addr, size);
		int result = _block_device->read(buffer, addr, size);
		_instrument.after_read(result, buffer, addr, size);
		return result;
	}

	virtual int program(const void *buffer, bd_addr_t addr, bd_size_t size) {
		_instrument.before_program(buffer, addr, size);
		int result = _block_device->program(buffer, addr, size);
		_instrument.after_program(result, buffer, addr, size);
		return result;
	}

	virtual int erase(bd_addr_t addr, bd_size_t size) {
		_instrument.before_erase(addr, size);
		int result = _block_device->erase(addr, size);
		_instrument.after_erase(result, addr, size);
		return result;
	}

private:
	BlockDevice* _block_device;
	BlockDeviceInstrument& _instrument;
};

It become possible to create any kind of new instrument and use it in the instrumented block device class:

// generic counter
class BlockDeviceInstrumentCounter : public BlockDeviceInstrument {
public:
	BlockDeviceInstrumentCounter() : counter(0) { }

	virtual void reset() {
		counter = 0;
	}

	size_t  get_counter() const {
		return counter;
	}

protected:
	size_t counter;
};

// instrument successful read
class BlockDeviceReadCounter : public BlockDeviceInstrumentCounter {
public:
	virtual void after_read(int result, void *buffer, bd_addr_t addr, bd_size_t size) {
		if (result == 0) {
			counter += size;
		}
	}
};

// instrument failure
class BlockDeviceReadFaillureCounter : public BlockDeviceInstrumentCounter {
public:
	virtual void after_read(int result, void *buffer, bd_addr_t addr, bd_size_t size) {
		if (result != 0) {
			++counter;
		}
	}
};

Switching from one type of instrumentation to another really just is changing the instrument injected in the InstrumentedBlockDevice at construction time:

void instrument_faillure(BlockDevice* block_device) { 
	BlockDeviceReadFaillureCounter faillure_counter;
	InstrumentedBlockDevice bd(block_device, faillure_counter);

	// do some work

	size_t faillures = faillure_counter.get_counter();
}

void instrument_read(BlockDevice* block_device) { 
	BlockDeviceReadCounter read_counter;
	InstrumentedBlockDevice bd(block_device, read_counter);

	// do some work

	size_t bytes_reads = read_counter.get_counter();
}

Composition of instruments can be handled by a composite of Instrument object:

struct CompositeBlockDeviceInstrument : BlockDeviceInstrument {
	template<size_t N>
	CompositeBlockDeviceInstrument(BlockDeviceInstrument* (&instruments)[N]) :
		_instruments(instruments), _count(N) { }

	virtual void before_read(void *buffer, bd_addr_t addr, bd_size_t size) {
		for (size_t i = 0; i < _count; ++i) {
			_instruments[i]->before_read(buffer, addr, size);
		}
	}
	virtual void after_read(int result, void *buffer, bd_addr_t addr, bd_size_t size) {
		for (size_t i = 0; i < _count; ++i) {
			_instruments[i]->after_read(result, buffer, addr, size);
		}
	}
	virtual void before_program(const void *buffer, bd_addr_t addr, bd_size_t size) {
		for (size_t i = 0; i < _count; ++i) {
			_instruments[i]->before_program(buffer, addr, size);
		}
	}

	virtual void after_program(int result, const void *buffer, bd_addr_t addr, bd_size_t size) {
		for (size_t i = 0; i < _count; ++i) {
			_instruments[i]->after_program(result, buffer, addr, size);
		}
	}

	virtual void before_erase(bd_addr_t addr, bd_size_t size) {
		for (size_t i = 0; i < _count; ++i) {
			_instruments[i]->before_erase(addr, size);
		}
	}

	virtual void after_erase(int result, bd_addr_t addr, bd_size_t size) {
		for (size_t i = 0; i < _count; ++i) {
			_instruments[i]->after_erase(result, addr, size);
		}
	}

	virtual void reset() {
		for (size_t i = 0; i < _count; ++i) {
			_instruments[i]->reset();
		}
	}

private:
	 BlockDeviceInstrument** _instruments;
	 size_t _count;
};

Composition is easy, cheap and extensible. Since there is no long inheritance chain, name clash are also avoided:

void instrument_read_and_faillure(BlockDevice* block_device) { 
	BlockDeviceReadFaillureCounter faillure_counter;
	BlockDeviceReadCounter read_counter;
	BlockDeviceInstrument* instruments_list[] = {
		&read_counter, &faillure_counter
	};
	CompositeBlockDeviceInstrument instruments(instruments_list);

	InstrumentedBlockDevice bd(block_device, instruments);

	// do some work

	size_t faillures = faillure_counter.get_counter();
	size_t bytes_read = read_counter.get_counter();
}

But what has been gained from architecturing the code:

• Respect of single responsibility principle and loose coupling: instrument do a single task, instrumentation, nothing more which makes them simple to reason about. A change in an instrument doesn't impact another one. Similarly, a change in the BlockDevice class doesn't impact the instrument.
• Extensibility: Adding a new instrument can be done in 10 lines of code and doesn't require the overriding of any function of the BlockDevice.
• Composability: Instrumentation is not part of the BlockDevice hierarchy nor a deep Instrument hierarchy, they can be easily mixed and composed.
• Testability: Testing the instrument does not require any working block device or even instrumentation to happen.

Note that it is also possible to avoid virtual functions and use static polymorphism: https://gist.github.com/pan-/b136a3d56200c0f1647d43d1315ed427 .

[AnotherButler]

@geky In general*, I don't have a problem with using a colon after a module as a prefix. However, in this case, it adds extra letters to a subject line that's already longer than the recommended length, and those letters don't add any value because we already know we're talking about BlockDevice.

*Today's release note has a few entries with multiple colons, which I do not like because it gets confusing.

[theotherjimmy]

Keeping a consistent module name is very useful for greping for related commits.

Would git log features/filesystem/bd not work well for this use case?

[pan-]

Keeping a consistent module name is very useful for greping for related commits. (We don't have an enforced policy so it's not thaaat useful, but would be nice to have ;) )

Actually I think we do have a policy: https://github.com/ARMmbed/mbed-os-morpheus/blob/guidelines/GUIDELINES.md#contribution .

[geky]

@jupe, that's a good idea, but write errors usually aren't detected at this level. It could be added later if we find it useful.

@pan-, that's neat 👍, though might be a bit overkill for this class in the little corner of the bd api. It's probably simpler to just shove any other metrics we want into this single class for now.

@AnotherButler, no tweaking is going to make that commit message fit in 50 characters aside from dropping "ProfilingBlockDevice", but as that's the class name and seems a bit important. The point of a consistent module name is to be consistent, and "bd" is already somewhat useful. Also it's only 3 letters, I could make the name even shorter by removing all of the whitespace.

@theotherjimmy, that is useful. But doesn't work when filtering commits in releases or github issues/prs.

@pan-, that's why I tossed in the word "enforced" ; ) . I'm sure we'll get there eventually.

[pan-]

Could you make these variable private ?

[geky]

Oh! I'm not sure how I didn't notice that, sure thing

[pan-]

@geky I know its overkill, especially if it is not needed right now however I wanted to stress out that virtuality and protected access by default, just in case, open the door to poor design. It is not a small decision to make a class part of the public API because it has to be maintained and supported once it is public.

Design patterns, best practices or programming principles (KISS, YAGNI, SOLID, ...) are not just fancy concepts but rather are valuable tools. A legacy of thousands of failures and success which can help us in delivering better software if applied with a pragmatic thinking.

[geky]

The member variables were never intended to be protected, I'm actually not sure how they got there in the first place.

Virtualness is a bit different. What concerns are there besides the runtime/memory cost?

[pan-]

Virtualness is a bit different. What concerns are there besides the runtime/memory cost?

First virtualness indicates that the function can be overridden and a feature of a class can be extended. This is not desirable for every function or for every class. Incorrect overloading; it is so easy to get it wrong. For example if one of the accessor is overloaded or a new one is added then the reset function has to be overloaded and it shall call the parent reset function.

The virtual specifier also indicates that derivation is the means chosen to extend class features. Is it the best way to add new profiling feature ?

[geky]

Ok, the point about mbed stating a preference for extending makes sense to me.

Does this pr look good as is?

[pan-]

@geky Yes it looks good 👍 .

[geky]

Cool beans : )

/morph test-nightly

[mbed-bot]

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 968

Test failed!

[studavekar]

profiling_of_block_devices tests seems to failing for all platform

[1502236490.84][CONN][RXD] >>> Running case #3: 'Testing profiling of block devices'...
[1502236490.90][CONN][INF] found KV pair in stream: {{__testcase_start;Testing profiling of block devices}}, queued...
[1502236490.94][CONN][RXD] :193::FAIL: Expected 4096 Was 8192
[1502236491.00][CONN][INF] found KV pair in stream: {{__testcase_finish;Testing profiling of block devices;0;1}}, queued...
[1502236491.10][CONN][RXD] >>> 'Testing profiling of block devices': 0 passed, 1 failed with reason 'Assertion Failed'

[geky]

Woops! nice catch
/morph test-nightly

[mbed-bot]

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1020

Test failed!

[geky]

/morph test-nightly

[mbed-bot]

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1024

Test failed!

[0xc0170]

/morph test-nightly

[mbed-bot]

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1032

All builds and test passed!