Skip to content

Commit d7a129d

Browse files
author
mikee47
committed
Merge branch 'feature/optimise' into develop
This PR improves performance of `FlashString` library object iteration and indexing. For example, calling `indexOf` on an `Array<uint32_t>` object results in repeated calls to the `data()` method. Further, the C++ iterator implementation does the same. A new speed test module has been added to evaulate performance. This uses some moderately large test objects and evaluates how long it takes to iterate through them using three methods: 1. regular `for(unsigned i=0; i<array.count(); ++i)` loop 2. C++ iterator `for(auto: array)` 3. Using the `indexOf` method Array<int> has 1000 elements. Vector<String> has 367 elements, referencing 3208 bytes of string data. Map<int, String> has 367 elements. These are the results, in microseconds, on an esp8266: | Operation | Current | With this PR | -------------------------------------- | ------------ | ------------ | | Array<int> for-loop | 1603 | 1611 | | Array<int> iterator | 1326 | 403 | | Array<int>.indexOf | 1303 | 425 | | Vector<String> for-loop | 800 | 721 | | Vector<String> iterator | 774 | 402 | | Vector<String> indexOf(const char*) | 6634 | 969 | | Vector<String> indexOf(String) | 849 | 476 | | Map<int, String> for-loop | 1042 | 1085 | | Map<int, String> iterator | 891 | 577 | | Map<int, String> indexOf | 189 | 189 | | Map<int, String> lookup | 168 | 168 | **Optimise iteration and `indexOf` operation** The regular `[]` operator and `valueAt` methods include range checks which are costly inside a loop. Adding `unsafe` methods skips these checks. **Improve Vector search for char* argument** The `indexOf` method has been specialised for `char*` arguments so that `strlen()` only needs to be called once. Previously it was called on every loop iteration. **Provide base `==` operator which does binary comparison** This allows easy searching by value (object content). **Rationalise string comparison methods** FlashStrings have three `equals` overloads for `char*`, `String` and `FlashString` arguments. We also require three `equalsignorecase` equivalents and appropriate `operator==` implementations. By adding an `ignoreCase` parameter to all three `equals` methods we can simplify code. **Other improvements** - Use const references instead of copy - Move copy/invalidate operations to constructors - Use constexpr where possible for method return values - Move `printElement(Print, char)` specialization into source file - Move `read` method out of header - Add github workflow - Put strings in flash
2 parents 4821edc + a6a74a2 commit d7a129d

22 files changed

+897
-199
lines changed

‎.github/workflows/ci-dispatch.yml

+20
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
name: CI Dispatch
2+
3+
on:
4+
workflow_dispatch:
5+
inputs:
6+
sming_repo:
7+
description: 'Full URL for Sming repository'
8+
default: 'https://github.com/SmingHub/Sming'
9+
type: string
10+
sming_branch:
11+
description: 'Sming branch to run against'
12+
default: 'develop'
13+
type: string
14+
15+
jobs:
16+
build:
17+
uses: SmingHub/Sming/.github/workflows/library.yml@develop
18+
with:
19+
sming_repo: ${{ inputs.sming_repo }}
20+
sming_branch: ${{ inputs.sming_branch }}

‎.github/workflows/ci-push.yml

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
name: CI Push
2+
3+
on: [push, pull_request]
4+
5+
jobs:
6+
build:
7+
uses: SmingHub/Sming/.github/workflows/library.yml@develop

‎src/ArrayPrinter.cpp

+76
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
/****
2+
* ArrayPrinter.cpp - Print support for arrays
3+
*
4+
* Copyright 2019 mikee47 <[email protected]>
5+
*
6+
* This file is part of the FlashString Library
7+
*
8+
* This library is free software: you can redistribute it and/or modify it under the terms of the
9+
* GNU General Public License as published by the Free Software Foundation, version 3 or later.
10+
*
11+
* This library is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY;
12+
* without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
13+
* See the GNU General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU General Public License along with this library.
16+
* If not, see <https://www.gnu.org/licenses/>.
17+
*
18+
****/
19+
20+
#include "include/FlashString/ArrayPrinter.hpp"
21+
22+
namespace FSTR
23+
{
24+
size_t printElement(Print& p, char c)
25+
{
26+
auto escape = [](char c) -> char {
27+
switch(c) {
28+
case '\0':
29+
return '0';
30+
case '\'':
31+
return '\'';
32+
case '\"':
33+
return '"';
34+
case '\?':
35+
return '?';
36+
case '\\':
37+
return '\\';
38+
case '\a':
39+
return 'a';
40+
case '\b':
41+
return 'b';
42+
case '\f':
43+
return 'f';
44+
case '\n':
45+
return 'n';
46+
case '\r':
47+
return 'r';
48+
case '\t':
49+
return 't';
50+
case '\v':
51+
return 'v';
52+
default:
53+
return '\0';
54+
}
55+
};
56+
57+
char buf[8];
58+
char* o = buf;
59+
*o++ = '\'';
60+
char esc = escape(c);
61+
if(esc) {
62+
*o++ = '\\';
63+
*o++ = esc;
64+
} else if(isprint(c)) {
65+
*o++ = c;
66+
} else {
67+
*o++ = '\\';
68+
*o++ = 'x';
69+
*o++ = hexchar(uint8_t(c) >> 4);
70+
*o++ = hexchar(uint8_t(c) & 0x0f);
71+
}
72+
*o++ = '\'';
73+
return p.write(buf, o - buf);
74+
}
75+
76+
} // namespace FSTR

‎src/ObjectBase.cpp

+18-18
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,17 @@ namespace FSTR
2525
const ObjectBase ObjectBase::empty_{ObjectBase::lengthInvalid};
2626
constexpr uint32_t ObjectBase::copyBit;
2727

28+
bool ObjectBase::operator==(const ObjectBase& other) const
29+
{
30+
if(this == &other) {
31+
return true;
32+
}
33+
if(length() != other.length()) {
34+
return false;
35+
}
36+
return memcmp(this, &other, sizeof(flashLength_) + size()) == 0;
37+
}
38+
2839
size_t ObjectBase::readFlash(size_t offset, void* buffer, size_t count) const
2940
{
3041
auto len = length();
@@ -37,15 +48,16 @@ size_t ObjectBase::readFlash(size_t offset, void* buffer, size_t count) const
3748
return flashmem_read(buffer, addr, count);
3849
}
3950

40-
size_t ObjectBase::length() const
51+
size_t ObjectBase::read(size_t offset, void* buffer, size_t count) const
4152
{
42-
if(isNull()) {
53+
auto len = length();
54+
if(offset >= len) {
4355
return 0;
44-
} else if(isCopy()) {
45-
return reinterpret_cast<const ObjectBase*>(flashLength_ & ~copyBit)->length();
46-
} else {
47-
return flashLength_;
4856
}
57+
58+
count = std::min(len - offset, count);
59+
memcpy_P(buffer, data() + offset, count);
60+
return count;
4961
}
5062

5163
const uint8_t* ObjectBase::data() const
@@ -71,16 +83,4 @@ const uint8_t* ObjectBase::data() const
7183
return reinterpret_cast<const uint8_t*>(&ptr->flashLength_ + 1);
7284
}
7385

74-
void ObjectBase::invalidate()
75-
{
76-
#ifndef ARCH_HOST
77-
// Illegal on real flash object
78-
assert(!isFlashPtr(this));
79-
if(isFlashPtr(this)) {
80-
return;
81-
}
82-
#endif
83-
flashLength_ = lengthInvalid;
84-
}
85-
8686
} // namespace FSTR

‎src/String.cpp

+26-21
Original file line numberDiff line numberDiff line change
@@ -23,32 +23,44 @@
2323

2424
namespace FSTR
2525
{
26-
bool String::equals(const char* cstr, size_t len) const
26+
bool String::equals(const char* cstr, size_t clen, bool ignoreCase) const
2727
{
2828
// Unlikely we'd want an empty flash string, but check anyway
2929
if(cstr == nullptr) {
3030
return length() == 0;
3131
}
32-
// Don't use strcmp as our data may contain nuls
33-
if(len == 0) {
34-
len = strlen(cstr);
35-
}
36-
if(len != length()) {
32+
auto len = length();
33+
if(clen != len) {
3734
return false;
3835
}
3936
LOAD_FSTR(buf, *this);
37+
if(ignoreCase) {
38+
return memicmp(buf, cstr, len) == 0;
39+
}
4040
return memcmp(buf, cstr, len) == 0;
4141
}
4242

43-
bool String::equals(const String& str) const
43+
bool String::equals(const char* cstr, bool ignoreCase) const
44+
{
45+
return equals(cstr, cstr ? strlen(cstr) : 0, ignoreCase);
46+
}
47+
48+
bool String::equals(const String& str, bool ignoreCase) const
4449
{
45-
if(data() == str.data()) {
50+
auto dataptr = data();
51+
auto strdata = str.data();
52+
if(dataptr == strdata) {
4653
return true;
4754
}
48-
if(length() != str.length()) {
55+
auto len = length();
56+
if(len != str.length()) {
4957
return false;
5058
}
51-
return memcmp_aligned(data(), str.data(), length()) == 0;
59+
if(ignoreCase) {
60+
LOAD_FSTR(buf, *this);
61+
return memicmp(dataptr, buf, len) == 0;
62+
}
63+
return memcmp_aligned(dataptr, strdata, len) == 0;
5264
}
5365

5466
/* Wiring String support */
@@ -58,25 +70,18 @@ String::operator WString() const
5870
return isNull() ? WString() : WString(data(), length());
5971
}
6072

61-
bool String::equals(const WString& str) const
73+
bool String::equals(const WString& str, bool ignoreCase) const
6274
{
6375
auto len = str.length();
6476
if(len != length()) {
6577
return false;
6678
}
6779
// @todo optimise memcmp_P then we won't need to load entire String into RAM first
6880
LOAD_FSTR(buf, *this);
69-
return memcmp(buf, str.c_str(), len) == 0;
70-
}
71-
72-
bool String::equalsIgnoreCase(const WString& str) const
73-
{
74-
auto len = str.length();
75-
if(len != length()) {
76-
return false;
81+
if(ignoreCase) {
82+
return memicmp(buf, str.c_str(), len) == 0;
7783
}
78-
LOAD_FSTR(buf, *this);
79-
return memicmp(buf, str.c_str(), len) == 0;
84+
return memcmp(buf, str.c_str(), len) == 0;
8085
}
8186

8287
} // namespace FSTR

‎src/include/FlashString/Array.hpp

+2
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,8 @@ namespace FSTR
113113
template <typename ElementType> class Array : public Object<Array<ElementType>, ElementType>
114114
{
115115
public:
116+
static_assert(!std::is_pointer<ElementType>::value, "Pointer types not supported by Array - use Vector");
117+
116118
/* Arduino Print support */
117119

118120
/**

‎src/include/FlashString/ArrayPrinter.hpp

+4-53
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/****
2-
* ArrayPrinter.cpp - Print support for arrays
2+
* ArrayPrinter.hpp - Print support for arrays
33
*
44
* Copyright 2019 mikee47 <[email protected]>
55
*
@@ -24,62 +24,13 @@
2424

2525
namespace FSTR
2626
{
27-
template <typename T> typename std::enable_if<!std::is_same<T, char>::value, size_t>::type printElement(Print& p, T e)
27+
template <typename T>
28+
typename std::enable_if<!std::is_same<T, char>::value, size_t>::type printElement(Print& p, const T& e)
2829
{
2930
return print(p, e);
3031
}
3132

32-
template <typename T> typename std::enable_if<std::is_same<T, char>::value, size_t>::type printElement(Print& p, T c)
33-
{
34-
auto escape = [](char c) -> char {
35-
switch(c) {
36-
case '\0':
37-
return '0';
38-
case '\'':
39-
return '\'';
40-
case '\"':
41-
return '"';
42-
case '\?':
43-
return '?';
44-
case '\\':
45-
return '\\';
46-
case '\a':
47-
return 'a';
48-
case '\b':
49-
return 'b';
50-
case '\f':
51-
return 'f';
52-
case '\n':
53-
return 'n';
54-
case '\r':
55-
return 'r';
56-
case '\t':
57-
return 't';
58-
case '\v':
59-
return 'v';
60-
default:
61-
return '\0';
62-
}
63-
};
64-
65-
char buf[8];
66-
char* o = buf;
67-
*o++ = '\'';
68-
char esc = escape(c);
69-
if(esc) {
70-
*o++ = '\\';
71-
*o++ = esc;
72-
} else if(isprint(c)) {
73-
*o++ = c;
74-
} else {
75-
*o++ = '\\';
76-
*o++ = 'x';
77-
*o++ = hexchar(uint8_t(c) >> 4);
78-
*o++ = hexchar(uint8_t(c) & 0x0f);
79-
}
80-
*o++ = '\'';
81-
return p.write(buf, o - buf);
82-
}
33+
size_t printElement(Print& p, char c);
8334

8435
/**
8536
* @brief Class template to provide a simple way to print the contents of an array

‎src/include/FlashString/MapPair.hpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -105,10 +105,10 @@ template <typename KeyType, class ContentType> class MapPair
105105

106106
if(*this) {
107107
count += print(p, key());
108-
count += p.print(" => ");
108+
count += p.print(_F(" => "));
109109
count += print(p, content());
110110
} else {
111-
count += p.print("(invalid)");
111+
count += p.print(_F("(invalid)"));
112112
}
113113

114114
return count;

0 commit comments

Comments
 (0)