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

Refactor linked list to lock-free ring buffer #6139

Closed
wants to merge 22 commits into from

Conversation

dok-net
Copy link
Contributor

@dok-net dok-net commented May 24, 2019

This amends #6039 to use a circular queue that is lock-free between producer/consumer and is tested on ESP8266, ESP32 and x86_64.

@d-a-v
Copy link
Collaborator

d-a-v commented May 24, 2019

Do you have other use-cases for this ring buffer ?

@dok-net
Copy link
Contributor Author

dok-net commented May 27, 2019

@d-a-v The queue is used in EspSoftwareSerial.

@dok-net
Copy link
Contributor Author

dok-net commented May 28, 2019

@d-a-v @devyte I've prepared a test sketch that stresses the queue implementation of the Scheduler.
Over 100000 scheduled tasks, in chunks of 30 [20] per ticker, the circular_queue-based scheduler requires just over 3.3s [5s] real time.
For the same workload, the current linked-list finishes after more than 43s [10s] real time.

In terms of quality, the circular queue suffers a performance degredation (24s real time) when the chunk size is increased to 50, this is expected behavior due to the many failed and repeated attempts to insert into the well exhausted queue of size 32.
Even at 35 schedules-per-ticker, though, the linked-list fails disastrously (Soft WDT reset).

Please check for yourself and comment if I've egregiously misses something:

// QueueTest.cpp : This file contains the 'main' function. Program execution begins and ends there.
//

#include <Ticker.h>
#include "PolledTimeout.h"

constexpr long unsigned MSGCNT = 100000;
constexpr long unsigned MSGSPERTICKER = 30;

int pending = 1;

long unsigned msgNo = 0;
long unsigned nextVal[] = { 0 };

uint32_t startMillis;

bool checkMessage(long unsigned msg)
{
	const auto id = msg & 0x1f;
	const auto val = msg >> 5;

	if (id < 0 || id >= 1) {
		String str = "received bad thread id = ";
		str += int(id);
		Serial.println(str);
		return true;
	}
	if (val == MSGCNT - 1) {
		--pending;
		return false;
	}

	if (val != nextVal[id])
	{
		if (val < nextVal[id]) {
			String str = "value in reverse, id = ";
			str += int(id);
			str += ", val = ";
			str += int(val);
			Serial.println(str);
			nextVal[id] = val + 1;
		}
		else {
			String str = "values skipped, id = ";
			str += int(id);
			str += ", lost = ";
			str += int(val - nextVal[id]);
			Serial.println(str);
			nextVal[id] = val + 1;
		}
	}
	else
	{
		nextVal[id] = val + 1;
	}
	return true;
}

Ticker ticker;

ICACHE_RAM_ATTR void tickerCallback(int)
{
	for (int i = 0; i < MSGSPERTICKER; ++i) {
		if (msgNo >= MSGCNT) return;
		auto msg = (msgNo << 5) + 0;

		if (schedule_function([msg]()
			{
				if (!checkMessage(msg)) { ticker.detach(); }
			}))
		{
			++msgNo;
		}
	}
}

void setup()
{
	Serial.begin(115200);
	delay(1000);
	Serial.println("scheduler test");
	startMillis = millis();

	if (schedule_function_us([]() { Serial.print("scheduler"); /*delay(2000);*/ Serial.println(" alive"); yield(); return true; }, 5000000))
	{
		Serial.println("scheduled 'scheduler alive' function");
	}
	else
	{
		Serial.println("could not schedule 'scheduler alive' function");
	}

	schedule_function_us([]() { Serial.print(".."); return true; }, 500000);

	ticker.attach_ms<int>(1, tickerCallback, 0);
}

esp8266::polledTimeout::periodicFastUs loopAlive(5000000);

void loop()
{
	if (pending == 0)
	{
		const auto val = nextVal[0];
		if (val != MSGCNT - 1)
		{
			String str = "received bad last message, id = ";
			str += 0;
			str += ", val = ";
			str += int(val);
			Serial.println(str);
		}

		Serial.print("done after ");
		Serial.print(millis() - startMillis);
		Serial.println("ms");

		pending = -1;
	}

	if (loopAlive) Serial.println("loop alive");
}

// x86_64:
// linked-list: 129340ms CPU usage,  57s wall clock
// linked-list:  93610ms CPU usage,  39s wall clock
// linked-list: 214984ms CPU usage, 100s wall clock
// circular q:   66317ms CPU usage,  30s wall clock
// circular q:   67909ms CPU usage,  32s wall clock
// circular q:   55780ms CPU usage,  25s wall clock
// linked-list: 193866ms CPU usage,  90s wall clock
// circular q:   70780ms CPU usage,  32s wall clock

// ESP8266:
// linked-list:  43904ms wall clock
// circular q:    3354ms wall clock

@d-a-v
Copy link
Collaborator

d-a-v commented May 28, 2019

Thanks for your work and your proofs/tests.
I will try it after the exposed API #6158 is approved by everyone.
I think you will need to add some comments in the class.
Since it is supposed to be used in this core and also in espsoftwareserial used by esp32 too, did you consider to put it in a separate submoduled repository so everybody can benefit from bugfixes from a single location ?

@dok-net
Copy link
Contributor Author

dok-net commented May 28, 2019

@d-a-v git submodules... but have a point here, certainly.
I've merged and fixed your notinyield branch with the circular queue, if you're interested this is in https://github.com/dok-net/Arduino/tree/notinyield

@dok-net dok-net force-pushed the recurrentscheduledfunctions branch from 97ef85b to 2a5777b Compare May 29, 2019 10:50
@dok-net
Copy link
Contributor Author

dok-net commented May 31, 2019

@d-a-v I've implemented the Ticker-based timing here - this eliminates the use of PolledTimeout except for the 100ms period yielding.

  • Are you dependent on microsecond resolution? The Ticker by default is in millisecond resolution mode and IIRC works at 5ms minimum delay.
  • The repeat value in this implementation is not continues, but always begins counting at the time the function was scheduled and returns to the scheduler - this adds to the imprecision...
  • Could you again give this implementation a run in your environment, please (if the above listed constraints don't forbid it straight off)? Porting the Ticker use to the linked-list version of Schedule is a no-brainer, I believe, for comparison purposes.

@dok-net
Copy link
Contributor Author

dok-net commented May 31, 2019

@dok-net
How is ticker going to call a function in cont stack while an external library is busy waiting on network ?

The ESP8266 already has convenience functions that call schedule_function() - thus letting the user code run in CONT, not SYS. I've adapted the schedule_function_us() in Schedule.(h|cpp) to use Ticker, but in such fashion that eventually the execution of the scheduled code happens when run_scheduled_functions() kicks in - in CONT. Please check my questions above.
As a working example, here's my test code adapted to your yield() loop:

#include <atomic>
#include <Schedule.h>

void setup()
{
	Serial.begin(115200);
	delay(1000);
	Serial.println("scheduler test");

	if (schedule_function_us([]() { Serial.print(".."); return true; }, 500000))
	{
		Serial.println("scheduled 'alive'");
	}
	else
	{
		Serial.println("could not schedule 'alive'");
	}
}

void loop()
{
	uint32_t deadline = millis() + 10000;
	Serial.println("external library now busy");
	while (deadline - millis() > 0) yield();
	Serial.println("external library finally gets out");
}

@dok-net dok-net force-pushed the recurrentscheduledfunctions branch from dfa18a7 to 83db501 Compare May 31, 2019 01:33
@dok-net
Copy link
Contributor Author

dok-net commented May 31, 2019

@d-a-v You might like the latest commit 5a8584a : now the PolledTimeout is used for delays/periods up to 5000µs, where polling is quite sane, I guess, whereas for longer delays above the 5000µs minimum of Ticker, the latter is used, alleviating the load on the scheduler of polling tasks that will not be runnable for quite a few cycles.

@dok-net
Copy link
Contributor Author

dok-net commented Jun 3, 2019

@d-a-v With SCHEDULED_FN_MAX_COUNT == 32, the Ticker heap now consumes an additional 1280 bytes RAM. Would you think a lower number of slots for Ticker objects can do the job? These Tickers are used only for time-scheduled functions at 5000µs period or longer...

@dok-net dok-net force-pushed the recurrentscheduledfunctions branch 2 times, most recently from 57dda0d to 02dbff2 Compare June 10, 2019 22:47
@dok-net dok-net force-pushed the recurrentscheduledfunctions branch 2 times, most recently from a979416 to 6af67c6 Compare June 21, 2019 12:02
@dok-net dok-net force-pushed the recurrentscheduledfunctions branch 4 times, most recently from b8e8c13 to 3cb6f18 Compare June 26, 2019 07:47
@dok-net dok-net force-pushed the recurrentscheduledfunctions branch from 3cb6f18 to b28837b Compare June 27, 2019 08:37
@dok-net
Copy link
Contributor Author

dok-net commented Jun 27, 2019

Note: feature branches rebase against master, never in ongoing work merge master. Master eventually merges feature branches, does not rebase on feature branches (why should it).
Experience: Learn about git rerere before starting the first rebase. Merging master into a feature branch brings master's change history into that branch, makes the feature branch unmanageable (large number of visible, unrelated commits from master), and almost always reverts master to some mixed old state when the feature branch is accepted and merged.

@dok-net dok-net force-pushed the recurrentscheduledfunctions branch from b28837b to 26f453a Compare June 27, 2019 18:29
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are conflicts now, do you want to reaolve now, or do you want to wait until the other attachInterrupt stuff is out of the way?

libraries/Schedule/src/circular_queue/circular_queue.h Outdated Show resolved Hide resolved
@dok-net dok-net force-pushed the recurrentscheduledfunctions branch 2 times, most recently from 33793e3 to c54ca52 Compare July 5, 2019 08:58
@dok-net
Copy link
Contributor Author

dok-net commented Jul 5, 2019

Rebase - done.
Refactoring circular_queue - tbd next.

@dok-net dok-net force-pushed the recurrentscheduledfunctions branch from bc052b6 to 256406a Compare July 5, 2019 11:02
@dok-net dok-net force-pushed the recurrentscheduledfunctions branch 4 times, most recently from 4ee7dd9 to b5a2583 Compare August 1, 2019 09:34
@dok-net dok-net force-pushed the recurrentscheduledfunctions branch from d8d777d to 3aceb03 Compare August 1, 2019 14:35
@dok-net
Copy link
Contributor Author

dok-net commented Aug 1, 2019

Preparation for this library is now in other, simpler PRs. This library is not strictly needed, as the functionality exists in Schedule. Portability to ESP32 makes it preferable to take this to a standalone library. Closing PR. Thanks for the review!

@dok-net dok-net closed this Aug 1, 2019
@dok-net dok-net deleted the recurrentscheduledfunctions branch August 1, 2019 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants