WString: Optimize a bit #7553

jjsuwa · 2020-08-27T00:48:28Z

move bodies of dtor, init() and charAt() to .h (implicitly inlined)
unify the description of initialization into one: init() (literally), and called from each ctors, invalidate() and move().
invert the SSO state logic in order to make init state zeroed (as a result, inlined init() insns: 4 to 3)

IROM size @ FSBrowser: 304836 -> 304596 (saved 240 from master)

earlephilhower

Thanks, but the PR has some problems. I'm very wary of changing String because it can end up affecting a lot of things down the line if we mess anything up.

Do you have any numbers for the changes here, in terms of code size or String ops/sec? I really doubt these changes have an appreciable size or performance difference.

cores/esp8266/WString.h

cores/esp8266/WString.cpp

earlephilhower · 2020-08-28T15:52:43Z

Can you please give some hard #s on the size difference for a few examples using this vs. master, and the runtime differences, too?

You could do a build and then objdump -T to get the function sizes to do it at a mcro-level, or you could build FSBrowser.ino which makes extensive use of Strings and check total bin size.

jjsuwa · 2020-08-28T17:21:56Z

FSBrowser.ino build results:

commit e53aa87 (master in Aug 26, 2020):
FSBrowser.ino.bin : 338304 bytes
- IROM : 305020
- IRAM : 26340
- DATA : 1272
- RODATA : 1512
- BSS : 25576
  objdump -t (excerpt) :

00000008 g     F .text._ZN6StringC2Ec	00000031 _ZN6StringC2Ec
00000008 g     F .text._ZN6StringC2Ec	00000031 _ZN6StringC1Ec
0000000c g     F .text._ZN6StringC2Ehh	0000003d _ZN6StringC2Ehh
0000000c g     F .text._ZN6StringC2Ehh	0000003d _ZN6StringC1Ehh
00000014 g     F .text._ZN6StringC2Eih	0000004f _ZN6StringC2Eih
00000014 g     F .text._ZN6StringC2Eih	0000004f _ZN6StringC1Eih
0000000c g     F .text._ZN6StringC2Ejh	0000003b _ZN6StringC2Ejh
0000000c g     F .text._ZN6StringC2Ejh	0000003b _ZN6StringC1Ejh
00000014 g     F .text._ZN6StringC2Elh	0000004f _ZN6StringC2Elh
00000014 g     F .text._ZN6StringC2Elh	0000004f _ZN6StringC1Elh
0000000c g     F .text._ZN6StringC2Emh	0000003b _ZN6StringC2Emh
0000000c g     F .text._ZN6StringC2Emh	0000003b _ZN6StringC1Emh
00000010 g     F .text._ZN6StringC2Efh	0000005b _ZN6StringC2Efh
00000010 g     F .text._ZN6StringC2Efh	0000005b _ZN6StringC1Efh
0000000c g     F .text._ZN6StringC2Edh	0000004e _ZN6StringC2Edh
0000000c g     F .text._ZN6StringC2Edh	0000004e _ZN6StringC1Edh
00000004 g     F .text._ZN6StringD2Ev	00000013 _ZN6StringD1Ev
00000004 g     F .text._ZN6StringD2Ev	00000013 _ZN6StringD2Ev
00000010 g     F .text._ZN6String4copyEPKcj	0000005b _ZN6String4copyEPKcj
00000010 g     F .text._ZN6String4copyEPK19__FlashStringHelperj	0000005b _ZN6String4copyEPK19__FlashStringHelperj

commit f4352b1d72180401f0669054f860d150cd025712 (this PR, latest):
FSBrowser.ino.bin : 338320 bytes
- IROM : 305036
- IRAM : 26340
- DATA : 1272
- RODATA : 1512
- BSS : 25576
  objdump -t (excerpt) :

00000008 g     F .text._ZN6StringC2Ec	0000002a _ZN6StringC2Ec
00000008 g     F .text._ZN6StringC2Ec	0000002a _ZN6StringC1Ec
00000010 g     F .text._ZN6StringC2Ehh	0000003d _ZN6StringC2Ehh
00000010 g     F .text._ZN6StringC2Ehh	0000003d _ZN6StringC1Ehh
00000018 g     F .text._ZN6StringC2Eih	00000051 _ZN6StringC2Eih
00000018 g     F .text._ZN6StringC2Eih	00000051 _ZN6StringC1Eih
00000010 g     F .text._ZN6StringC2Ejh	0000003b _ZN6StringC2Ejh
00000010 g     F .text._ZN6StringC2Ejh	0000003b _ZN6StringC1Ejh
00000018 g     F .text._ZN6StringC2Elh	00000051 _ZN6StringC2Elh
00000018 g     F .text._ZN6StringC2Elh	00000051 _ZN6StringC1Elh
00000010 g     F .text._ZN6StringC2Emh	0000003b _ZN6StringC2Emh
00000010 g     F .text._ZN6StringC2Emh	0000003b _ZN6StringC1Emh
00000014 g     F .text._ZN6StringC2Efh	00000055 _ZN6StringC2Efh
00000014 g     F .text._ZN6StringC2Efh	00000055 _ZN6StringC1Efh
00000010 g     F .text._ZN6StringC2Edh	00000045 _ZN6StringC2Edh
00000010 g     F .text._ZN6StringC2Edh	00000045 _ZN6StringC1Edh
00000004 g     F .text._ZN6StringD2Ev	0000001c _ZN6StringD1Ev
00000004 g     F .text._ZN6StringD2Ev	0000001c _ZN6StringD2Ev
00000010 g     F .text._ZN6String4copyEPKcj	00000059 _ZN6String4copyEPKcj
00000010 g     F .text._ZN6String4copyEPK19__FlashStringHelperj	00000059 _ZN6String4copyEPK19__FlashStringHelperj

devyte · 2020-08-28T17:27:40Z

That shows that your optimization attempts are going the wrong way: 16 bytes increase in IROM.

jjsuwa · 2020-08-28T18:40:27Z

commit cb6dca9df0e677f336f842660de1a61eed705d75 (this PR: latest):
FSBrowser.ino.bin : 338288 bytes
- IROM : 305004
- IRAM : 26340
- DATA : 1272
- RODATA : 1512
- BSS : 25576
  objdump -t (excerpt) :

00000008 g     F .text._ZN6StringC2Ec	00000029 _ZN6StringC2Ec
00000008 g     F .text._ZN6StringC2Ec	00000029 _ZN6StringC1Ec
0000000c g     F .text._ZN6StringC2Ehh	00000033 _ZN6StringC2Ehh
0000000c g     F .text._ZN6StringC2Ehh	00000033 _ZN6StringC1Ehh
00000014 g     F .text._ZN6StringC2Eih	00000047 _ZN6StringC2Eih
00000014 g     F .text._ZN6StringC2Eih	00000047 _ZN6StringC1Eih
0000000c g     F .text._ZN6StringC2Ejh	00000031 _ZN6StringC2Ejh
0000000c g     F .text._ZN6StringC2Ejh	00000031 _ZN6StringC1Ejh
00000014 g     F .text._ZN6StringC2Elh	00000047 _ZN6StringC2Elh
00000014 g     F .text._ZN6StringC2Elh	00000047 _ZN6StringC1Elh
0000000c g     F .text._ZN6StringC2Emh	00000031 _ZN6StringC2Emh
0000000c g     F .text._ZN6StringC2Emh	00000031 _ZN6StringC1Emh
00000010 g     F .text._ZN6StringC2Efh	0000004b _ZN6StringC2Efh
00000010 g     F .text._ZN6StringC2Efh	0000004b _ZN6StringC1Efh
0000000c g     F .text._ZN6StringC2Edh	0000003b _ZN6StringC2Edh
0000000c g     F .text._ZN6StringC2Edh	0000003b _ZN6StringC1Edh
00000004 g     F .text._ZN6StringD2Ev	0000001c _ZN6StringD2Ev
00000004 g     F .text._ZN6StringD2Ev	0000001c _ZN6StringD1Ev
00000010 g     F .text._ZN6String4copyEPKcj	00000059 _ZN6String4copyEPKcj
00000010 g     F .text._ZN6String4copyEPK19__FlashStringHelperj	00000059 _ZN6String4copyEPK19__FlashStringHelperj

devyte

Nothing jumps out at me.

HOWEVER: we've been bitten before by innocent-seeming minor tweaks here. Please go over the current unit tests covering the String class and see if you can find any uncovered code that is affected by your changes, and report back here.
If you find uncovered code, or uncovered corner cases, please add tests to ensure correct behavior.

cores/esp8266/WString.h

TD-er · 2020-08-31T07:40:28Z

cores/esp8266/WString.h

        union {
            struct _ptr ptr;
            struct _sso sso;
        };
+        static constexpr auto CAPACITY_MAX = (1U << sizeof(ptr.cap) * 8) - 1;


This is not used in the .h file, so why not move it to the .cpp?
Or could it be used from other classes? (it is protected, so only those inheriting from this class)

(it is protected, so only those inheriting from this class)

just only, but still possible.
if so, we should warn: [API change] (of course .isNotSSO too)

i'd like to know why these internal implementation members are protected but not private.

Well the init() function was really needed for proper functioning of the class, so if someone wants to make a derived class, this function must be protected (or public).
I can imagine it is not needed for using the String class, so then protected makes perfect sense.
Now init() is no longer needed in the current implementation, but it may indeed break API design if the function is removed. The same for the CAPACITY_MAX value.

TD-er · 2020-08-31T07:59:38Z

Is the init() function still needed?
Its declaration is already protected and it is only used now in the invalidate function.

devyte · 2020-08-31T15:44:12Z

cores/esp8266/WString.cpp

@@ -159,7 +159,8 @@ unsigned char String::changeBuffer(unsigned int maxStrLen) {
    // Fallthrough to normal allocator
    size_t newSize = (maxStrLen + 16) & (~0xf);
    // Make sure we can fit newsize in the buffer
-    if (newSize > CAPACITY_MAX) {
+    size_t maxCapacity = (1U << sizeof(ptr.cap) * 8) - 1;


Please be careful when refactoring, the original definition was static constexpr auto. At least make this constexpr.

thanks for the advice, @devyte

i have periodically checked the disassembled binaries using objdump -d.
in this case, the same as before.

cores/esp8266/WString.cpp

devyte · 2020-08-31T15:54:24Z

cores/esp8266/WString.h

-            unsigned char isSSO : 1;
+            unsigned char len      : 7; // Ensure only one byte is allocated by GCC for the bitfields
+            unsigned char isNotSSO : 1;
+            _sso() : len(0), isNotSSO(0) {


It has been proposed to move in the opposite direction: direct member init instead of constructor member init sequence.

direct member init for bitfield requires c++20, will bring some of CI tests failure (possibly host emulation)

That's not what I meant. You should be able to do direct member init below in the union in some form or other, i. e. {0}

For bitfield members that's not possible, as that's one of the new features of C++20.
So for this special case of the bitfields you can only initialize them in the constructor of the _sso struct.

As a quick test:

union { _sso sso = {{0},{0}}; _ptr ptr; };

works fine with C++11.

thx @devyte:

that's surely fine, in terms of the logic: wholly zeroed, due to funccall of memset(this, 0, sizeof(_sso)).
zeroing _sso.buff[0] and _sso.bitfields would be sufficient (and of course most efficient).

both class member initializers (char member = 0;) and braces initialization (struct _foo foo = { 0, ... };) cause whole initialization
(assumed that each of unspecified members has default value).

member initializer (: bar(0), ...) seems to be only way to realize sparse(holed) initialization.

The sso struct is 12 bytes in size, yet you seem keen on doing microoptimizations here. I don't consider this worthwhile, the only case where this would be significant is in a tight loop where >90% of the time is spent in initing a String object, and in such a case I would likely yell at the author. Personally, I think there are other places where spending time and effort would bring far better returns.
If you decide to pursue this, please:

find bin size for initialization with {{0},{0}}

compare against bin size with your member initializer

compare runtime performance of both cases

report back here

BTW, we're already on gcc 10.x. Although we have C++17 enabled, in theory we have access to C++20 experimental features.

BTW, we're already on gcc 10.x. Although we have C++17 enabled, in theory we have access to C++20 experimental features.

It was already mentioned before, this class is also ported to ESP32.
Are they also using gcc 10.x?
To be honest, if the braces initialization also works, then I would say to stick with what's working and not forcing to only be able to compile with the newest C++ standard.

What I'm more worried about is how the initialization is done when in a union.
In other words, what will be guaranteed to be initialized with the isSSO set to 1 (or the inverted one set to 0)?
We can talk about semantics and how the code may be the most aestetic pleasing to the reader.
But in the end it is important this one flag is always default set to mean isSSO is active.
If that should be done via automatic zero'ed initialization (thus inverting the flag) or the suggested:

union { _sso sso = {{0},{0}}; _ptr ptr; };

That's all fine by me.
Most important is that this isSSO is in its 'enabled' state (which isn't with this suggested code by the way) as default member value.

devyte · 2020-08-31T16:06:02Z

cores/esp8266/WString.h

-            unsigned char len   : 7; // Ensure only one byte is allocated by GCC for the bitfields
-            unsigned char isSSO : 1;
+            unsigned char len      : 7; // Ensure only one byte is allocated by GCC for the bitfields
+            unsigned char isNotSSO : 1;


What is this? Why the change to reverse logic? I see in addition that this is used as if(! isSSO()) in code, so this is a reverse of a reversed name that reverses a bit.
If there is a good reason for reversing, then please change the name to something that reflects the new meaning without the Not. Having to look at things like e. g. !isSSO() and then bool isSSO() {return !isNotSSO;} is just painful.
And I won't even mention the setSSO() body ....

hmm, unsigned char disabled : 1; or something, might be unambiguous.

If you need a better term, perhaps "heapBuffer" ? (isSSO was about not using a heap allocated buffer)

Before moving forward with any change, please explain why it needs to change at all.

I think its easy: the default constructors are setting "SSO" to true. by making the default "false" or "0", the initialization can be similified because almost all compilers and all architectures have some kind of loophole optimisation to find the best possible code pattern for initializing a variable or a set of variables to zero.

I think instead of using "SSO" which is an acronym that is hard to understand in the first place, I would suggest to call it "HeapString" or "isHeapString" or "isLargeString" . "isnotSSO" which is "is not small string optimisation" is really awkward english.

devyte · 2020-08-31T16:14:11Z

cores/esp8266/WString.h

            char *   buff;
            uint16_t cap;
            uint16_t len;
        };
        // This allows strings up up to 11 (10 + \0 termination) without any extra space.
-        enum { SSOSIZE = sizeof(struct _ptr) + 4 - 1 }; // Characters to allocate space for SSO, must be 12 or more
+        static constexpr auto SSOSIZE = sizeof(struct _ptr) + 4 - 1; // Characters to allocate space for SSO, must be 12 or more


The reason for the enum was that we were certain that way didn't create a constant with the value with old gcc compiler, even when using the value in code. We checked with sizeof and looked at the bin dump.
Are you positive that there is no case where this new definition creates a constant that increases the mem footprint?

as you indicated, this change has no performance effect (either better or worse).
according to the principle of minimum action, leaving it alone is probably the right way, i understood.

devyte · 2020-08-31T22:16:42Z

cores/esp8266/WString.cpp

-    buf[0] = c;
-    buf[1] = 0;
-    *this = buf;
+    uint16_t buf = __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ ? (uint16_t)c : (uint16_t)c << 8;


This looks duplicated, please refactor.

devyte · 2020-08-31T22:17:07Z

cores/esp8266/WString.cpp

-    buf[0] = c;
-    buf[1] = 0;
-    return concat(buf, 1);
+    uint16_t buf = __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ ? (uint16_t)c : (uint16_t)c << 8;


Duplicate, please refactor

I think also that most compilers perform this optimisation automatically. there might not be an observable behavior difference between old and new code.

earlephilhower · 2020-09-02T15:59:35Z

Guys, I'm seeing this PR morphing into two things.

An explicit init of the SSO/PTR union to catch potential OOM invalid frees (ie. "bug fix")
Some tweaks to the constructors to save +/- a few bytes but with no logical changes

Number 1 seems like a fine change and should be a 2-line PR, while number 2 is big and introduces issues of taste ans style.

Maybe the best way forward is to drop any functional changes from this and open a new one with only the "init sso/ptr union" change? That init PR should be easily approvable and get us runtime in end users to see if it has any effect.

devyte · 2020-09-02T17:03:07Z

I agree, let's split the "bug fix" off into its own pr to discuss.

…) stray dtor calls derived from esp8266#7553

TD-er · 2020-09-10T06:18:53Z

cores/esp8266/WString.h

+            struct {
+                char _offset0 = 0; // wbuffer()[0] = 0;
+                char _offset1[SSOSIZE - 1];
+                char _offset11 = 0; // setSSO(true); setLen(0);


I get why you want to do this, but I think the naming is a bit strange here.
Also, why not just use a constructor in one (not both) of the existing structs _ptr or _sso ?

I think _sso is the best choice here for having the constructor to zero all values.
And since it has to be done in the body of the constructor anyway, you can also make a function in that struct and call it both from its constructor and from the String::init() function.

As done here, using an unnamed struct with _offsetNN names is confusing.
For example _offset11 is actually the same address as used by the _sso flags.

TD-er · 2020-09-24T07:02:22Z

cores/esp8266/WString.h

@@ -68,7 +71,9 @@ class String {
        explicit String(unsigned long, unsigned char base = 10);
        explicit String(float, unsigned char decimalPlaces = 2);
        explicit String(double, unsigned char decimalPlaces = 2);
-        ~String(void);
+        inline ~String() {


Just curious, but what extra does inline add to a function implemented in a .h file?
I always thought implementing a function in a .h file always implies inline.

as you said, function bodies inside a class definition (typically in .h) are implicitly marked as inline.

like extra arithmetic parentheses, may be removed later.

mcspr

@jjsuwa also note of the #7611 / d3b3a43

Adding this test to tests/host/core/test_string.cpp fails on master, but not with this patch:

...

TEST_CASE("String::move", "[core][String]")
{
    const char buffer[] = "this string goes over the sso limit";

    String target;
    String source(buffer);

    target = std::move(source);
    REQUIRE(source.c_str() != nullptr);
    REQUIRE(target == buffer);
}

...

Perhaps you could add this test (or my commit) here?

re ALL: all existing change requests seem resolved, please re-review? I do believe initialization issue discussed above was resolved through the use of init(). Naming might not, but idk if it is still relevant.

earlephilhower · 2020-09-26T17:13:45Z

What is the new code size difference with these changes, @jjsuwa ?

The changing polarity of the isSSO bit into isHeap does not affect any actual code, it's just an unnecessary change IMO which makes things obtuse since the code is still using the name SSO in method names. Saving 3-4 bytes of flash would not be worth that obfuscation IMO (at the level of build noise). If the change saves 100 bytes or more, then it's meaningful.

earlephilhower

Can you also please add in @mcspr 's add'l String test which passes w/the fixed/minimized ::move(), too?

earlephilhower · 2020-09-26T17:15:17Z

cores/esp8266/WString.cpp

-    rhs.setLen(0);
-    rhs.setBuffer(nullptr);
+    invalidate();
+    sso = rhs.sso; // memcpy(this, &rhs, sizeof(*this));


I think this comment would be best removed.

earlephilhower · 2020-09-26T17:17:16Z

cores/esp8266/WString.cpp

@@ -225,32 +215,9 @@ String & String::copy(const __FlashStringHelper *pstr, unsigned int length) {

 #ifdef __GXX_EXPERIMENTAL_CXX0X__
 void String::move(String &rhs) noexcept {


Very nice cleanup of code here!

based on the esp8266#7553 without isSSO -> isHeap rename and inline optimizations additionally, remove useless pre-c++11 preprocessor checks Co-authored-by: Takayuki 'January June' Suwa <[email protected]>

* (test) WString: c_str() returns null pointer target = std::move(source) does not reset buffer pointer back to the sso * wstring: correctly do move invalidation & copy based on the #7553 without isSSO -> isHeap rename and inline optimizations additionally, remove useless pre-c++11 preprocessor checks Co-authored-by: Takayuki 'January June' Suwa <[email protected]>

d-a-v

Changes do not reflect the long discussion in this PR.
However approving the proposed changes as they are today.

Jason2866 · 2020-10-21T12:53:14Z

Nice one. Saves 544 bytes when used with Tasmota. Without 609844 bytes. With 609300 bytes.

dirkmueller

looks good to me. Oh please, please pretty please.

Tasmota Arduino Core v2.7.4.5 allowing webpassword over 47 characters (#9687)

earlephilhower

-edit- latest push has add'l changes that need to be reviewed before re-approval.

While I'm not that big of a fan of the isSSO->isHeap conversion, what's left here does seem legit, minimal, functional, and did have a measurable impact on other large projects, so approving. Thx!

d-a-v · 2020-10-29T11:42:08Z

cores/esp8266/WString.h

-        inline const char *buffer() const { return (const char *)(isSSO() ? sso.buff : ptr.buff); }
-        inline char *wbuffer() const { return isSSO() ? const_cast<char *>(sso.buff) : ptr.buff; } // Writable version of buffer
+        const char *buffer() const { return isSSO() ? sso.buff : ptr.buff; }
+        char *wbuffer() const { return const_cast<char *>(buffer()); } // Writable version of buffer


Instead of const_cast<> I would do the more readable other way around:

const char* buffer() const { return wbuffer(); } char* wbuffer() const { return isSSO() ? sso.buff : ptr.buff; } // Writable version of buffer

(I wonder why the compiler don't complain against the const in char* wbuffer() const)

(I wonder why the compiler don't complain against the const in char* wbuffer() const)

You don't change the pointer, only where the pointer points to?
Otherwise you would need something like const char* const wbuffer() const, which is not open for debate on ugliness IMHO ;)

cores/esp8266/WString.cpp

earlephilhower · 2020-11-02T02:49:44Z

cores/esp8266/WString.cpp

-    wbuffer()[fromIndex + 1] = '\0';
-    char* temp = strrchr(wbuffer(), ch);
-    wbuffer()[fromIndex + 1] = tempchar;
+    char *wbuffer = this->wbuffer();


The local variable wbuffer is the same as the class method wbuffer() so I'd really ask to change this name to something else.

I think the logic is still correct, it's just a readability and sanity thing.

earlephilhower · 2020-11-02T02:51:22Z

cores/esp8266/WString.cpp

-    wbuffer()[right] = '\0';
-    out = wbuffer() + left;  // pointer arithmetic
-    wbuffer()[right] = temp;  //restore character
+    char *wbuffer = this->wbuffer();


Same comment as above.

Also, for these two changes have you seen any code size difference? wbuffer() is a pretty simple call and I think it has a good chance of being inlined to a simple reference to the string array...

Jason2866 · 2020-11-02T15:28:10Z

@earlephilhower the last change does indeed save some bytes more.
With this change we have a 72 bytes smaller Tasmota build

d-a-v · 2020-11-10T09:02:06Z

In order for this PR to finally reach merging state, and thanks for this, can you please, as suggested by @earlephilhower rename char *wbuffer = this->wbuffer(); to avoid name collision ?
I'd suggest char *writableBuffer. But it could be writableb, or wb, writb, iamdifferentfromwbuffer or even just wbuf.
Well... I guess you got it @jjsuwa :)

edit Sorry I have been confused by the "forced-push".

@earlephilhower @devyte I guess your reviews were addressed.

* move bodies of dtor, `init()` and `charAt()` to .h (implicitly inlined) * unify descriptions of the initialization into one: `init()` (literally), that is called from each ctors, `invalidate()` and `move()` * invert the SSO state logic in order to make init state zeroed (as a result, each inlined `init()` saves 1 insn) * detab and trim * remove `inline` from .h * cosmetics * optimize the non-SSO -> SSO transition part of `changeBuffer()` * remove duped body of `operator =(StringSumHelper &&rval)` * remove common subexpressions from `lastIndexOf()` and `substring()` * eliminate `strlen(buf)` after calling `sprintf(buf, ...)` that returns # of chars written * eliminate `len()` after calling `setLen(newlen)` * make ctor`(char c)` inlineable * optimize `setLen()` * replace constant-forwarding overload functions with default argument ones * optimize `concat(char c)` IROM size @ FSBrowser: 304916 -> 304372 (saved 544 from master)

earlephilhower

I see the local variable wbuffer was renamed, thanks! We should put this in and then see if we have any issues at end users, then in a month or so submit the same change to the ESP32 repo.

IROM size @ FSBrowser: 304372 -> 304356 (saved 560 from master)

TD-er · 2020-11-15T11:59:28Z

cores/esp8266/WString.h

-            sso.len     = 0;
-            sso.isHeap  = 0;
+        void init(void) __attribute__((always_inline)) {
+            sso.buff[0]  = 0; // In the Xtensa ISA, in fact, 32-bit store insn ("S32I.N") is one-byte shorter than 8-bit one ("S8I").


About the comment; One byte or one bit shorter?
Or do you mean the generated binary code?

devyte · 2020-11-16T14:57:06Z

@jjsuwa What was the total gain from this optimization effort? Flash size, IRAM, heap, speed etc. Could you please redo your checks from end of August and report here for completeness?

jjsuwa-sys3175 · 2020-11-16T18:46:20Z

sorry, now @jjsuwa is unable to login due to my mistake (DNS MX expiration)...

@devyte :

Could you please redo your checks from end of August and report here for completeness?

end of August... such a long ago :)

FSBrowser.ino build results:

commit faf59f5, "Update to GCC 10.2 (Update to GCC 10.2 #7607)" (Sep 24, 2020):
FSBrowser.ino.bin : 338512 bytes
- IROM : 305252
- IRAM : 26324
- DATA : 1272
- RODATA : 1512
- BSS : 25552
commit faf59f5 + this PR (replacing core/esp8266/WString.*):
FSBrowser.ino.bin : 337744 bytes
- IROM : 304484
- IRAM : 26324
- DATA : 1272
- RODATA : 1512
- BSS : 25552

for speed... i guess, at least it shouldn't become slow by this PR because of code shrinkage, funccall overhead elimination and so on.

earlephilhower requested changes Aug 28, 2020

View reviewed changes

cores/esp8266/WString.h Outdated Show resolved Hide resolved

cores/esp8266/WString.cpp Outdated Show resolved Hide resolved

cores/esp8266/WString.cpp Show resolved Hide resolved

cores/esp8266/WString.cpp Show resolved Hide resolved

devyte requested changes Aug 28, 2020

View reviewed changes

TD-er reviewed Aug 30, 2020

View reviewed changes

cores/esp8266/WString.h Show resolved Hide resolved

TD-er reviewed Aug 31, 2020

View reviewed changes

devyte requested changes Aug 31, 2020

View reviewed changes

jjsuwa force-pushed the WString_cutWaste branch from f208e3c to b5a8527 Compare September 7, 2020 20:58

jjsuwa added a commit to jjsuwa/Arduino that referenced this pull request Sep 10, 2020

WString: make to be safer init state, in order to tolerate (potential…

aaddf0a

…) stray dtor calls derived from esp8266#7553

jjsuwa mentioned this pull request Sep 10, 2020

WString: make to be safer init state, in order to tolerate (potential) stray dtor calls #7590

Closed

TD-er reviewed Sep 10, 2020

View reviewed changes

jjsuwa force-pushed the WString_cutWaste branch from 3fbf724 to b39d016 Compare September 10, 2020 08:33

jjsuwa force-pushed the WString_cutWaste branch from b39d016 to dcbfd05 Compare September 24, 2020 06:22

TD-er reviewed Sep 24, 2020

View reviewed changes

jjsuwa force-pushed the WString_cutWaste branch 2 times, most recently from f3a315e to d018ce4 Compare September 25, 2020 00:20

mcspr mentioned this pull request Sep 25, 2020

WString: c_str() returns null pointer after move #7611

Merged

mcspr approved these changes Sep 25, 2020

View reviewed changes

earlephilhower self-requested a review September 26, 2020 17:03

earlephilhower requested changes Sep 26, 2020

View reviewed changes

jjsuwa force-pushed the WString_cutWaste branch from 62a5a88 to a07ec5c Compare September 27, 2020 20:34

d-a-v approved these changes Oct 15, 2020

View reviewed changes

dirkmueller approved these changes Oct 21, 2020

View reviewed changes

Jason2866 referenced this pull request in arendst/Tasmota Oct 22, 2020

Tasmota Arduino Core v2.7.4.5

eea472f

Tasmota Arduino Core v2.7.4.5 allowing webpassword over 47 characters (#9687)

earlephilhower requested review from earlephilhower and devyte October 28, 2020 14:23

earlephilhower approved these changes Oct 28, 2020

View reviewed changes

jjsuwa force-pushed the WString_cutWaste branch from bdb7b32 to fa0ad4d Compare October 29, 2020 03:29

earlephilhower requested review from earlephilhower and d-a-v October 29, 2020 03:32

d-a-v reviewed Oct 29, 2020

View reviewed changes

earlephilhower requested changes Nov 2, 2020

View reviewed changes

Jason2866 mentioned this pull request Nov 6, 2020

Change compile option to no-target-align arendst/Tasmota#9749

Merged

6 tasks

mcspr mentioned this pull request Nov 9, 2020

commit #7611 breaks compiler optimisation #7698

Closed

1 task

jjsuwa force-pushed the WString_cutWaste branch from 0294240 to f879080 Compare November 9, 2020 22:21

jjsuwa force-pushed the WString_cutWaste branch from f879080 to f2c7565 Compare November 13, 2020 23:47

earlephilhower approved these changes Nov 14, 2020

View reviewed changes

* optimize init() more

6e76a3a

IROM size @ FSBrowser: 304372 -> 304356 (saved 560 from master)

TD-er reviewed Nov 15, 2020

View reviewed changes

jjsuwa-sys3175 and others added 2 commits November 16, 2020 04:45

* revise the comment

ebe4ceb

Merge branch 'master' into WString_cutWaste

df10fff

d-a-v merged commit 8fe80f1 into esp8266:master Nov 16, 2020

jjsuwa deleted the WString_cutWaste branch November 16, 2020 14:43

mcspr mentioned this pull request Nov 18, 2020

esp8266: share port 80 with regular webserver Links2004/arduinoWebSockets#575

Merged

		@@ -225,32 +215,9 @@ String & String::copy(const __FlashStringHelper *pstr, unsigned int length) {

		#ifdef __GXX_EXPERIMENTAL_CXX0X__
		void String::move(String &rhs) noexcept {

WString: Optimize a bit #7553

WString: Optimize a bit #7553

Conversation

jjsuwa commented Aug 27, 2020 • edited Loading

earlephilhower left a comment

Choose a reason for hiding this comment

earlephilhower commented Aug 28, 2020

jjsuwa commented Aug 28, 2020

devyte commented Aug 28, 2020

jjsuwa commented Aug 28, 2020

devyte left a comment

Choose a reason for hiding this comment

TD-er Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TD-er commented Aug 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devyte Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

earlephilhower commented Sep 2, 2020

devyte commented Sep 2, 2020

TD-er Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcspr left a comment

Choose a reason for hiding this comment

earlephilhower commented Sep 26, 2020

earlephilhower left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-a-v left a comment

Choose a reason for hiding this comment

Jason2866 commented Oct 21, 2020

dirkmueller left a comment

Choose a reason for hiding this comment

earlephilhower left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jason2866 commented Nov 2, 2020

d-a-v commented Nov 10, 2020 • edited Loading

earlephilhower left a comment

Choose a reason for hiding this comment

TD-er Nov 15, 2020 • edited Loading

Choose a reason for hiding this comment

devyte commented Nov 16, 2020

jjsuwa-sys3175 commented Nov 16, 2020

jjsuwa commented Aug 27, 2020 •

edited

Loading

TD-er Aug 31, 2020 •

edited

Loading

devyte Aug 31, 2020 •

edited

Loading

TD-er Sep 10, 2020 •

edited

Loading

earlephilhower left a comment •

edited

Loading

d-a-v commented Nov 10, 2020 •

edited

Loading

TD-er Nov 15, 2020 •

edited

Loading