- Notifications
You must be signed in to change notification settings - Fork13.3k
Refactor Web Server Parsing-impl.h using StreamString to avoid String Reallocations#9005
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Should it count |
Hello@mcspr, Thanks for the suggestion, yours is more optimized. I will edit the PR now. |
I went into another rabbit hole as I was getting The Main culprit in the String Allocations is I only made the modification to the webserver, a lot of legacy code in the core that depends on |
Please go ahead, and thanks ! |
Given#9011, would you think having |
Lan-Hekary commentedNov 8, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@d-a-v Yes, think it would help too much. Upon further look of#9011 the implementation is based on character comparison, and I think it would not hurt performance. |
Right. It was made from |
Hello@d-a-v , |
| @@ -288,6 +289,59 @@ String Stream::readStringUntil(const char* terminator, uint32_t untilTotalNumber | |||
| return ret; | |||
| } | |||
| String Stream::readStreamString(const ssize_t maxLen ,const oneShotMs::timeType timeoutMs) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is exactly the same as::sendSize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not Exactly, It constructs a String Object and a Stream Object attached to it, then populate it, then returns the String Object, it's behavior is more like::readString
`sendSize' expect you to pass a Pointer to a Stream and returns the total number of bytes.
readStreamString is just a wrapper for the String and Stream contruction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is indeed not exactly the same.
The goal of thesend* functions is to handle and accelerate transfers for any kind of stream like StreamString are.
(edited:)
line=client.readStreamString(size);// (no copy)
can also be written as
client.sendSize(S2Stream(line),size);// no copy (not tried though, meant to work, temporary is assumed)
Anyway this function is not used in the following of your pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, your version is much smaller 😄
My version is not used indeed, but made it available so that we have a Uniform API like 'readString*' but with streams.
Please advise on the best course of action, should I remove the unused functions ?
Do you have a better Idea to make the API more intuitive ?
| return ret; | ||
| } | ||
| String Stream::readStreamStringUntil(const int readUntilChar, const oneShotMs::timeType timeoutMs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same as::sendUntil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Based on::readStringUntil
| return ret; | ||
| } | ||
| String Stream::readStreamStringUntil (const char* terminatorString, uint32_t untilTotalNumberOfOccurrences, const oneShotMs::timeType timeoutMs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For full genericity with any kind of stream, we should rather extend::sendGeneric to accept achar* instead of achar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We Should Do that, but I am having a hard time making this modification.
This Function will be much simpler if we can do that.
But The basic Idea here is to match the API ofreadStringUntil for single char in the first function and Terminator String in this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For full genericity with any kind of stream, we should rather extend::sendGeneric to accept a char* instead of a char.
Having tried implementing that...
- we'd have to change API from char to cstr*?
- 'basic' streams have to have some kind of logic jump to stall writes until delimiter is actually read, since the current one simply pushes char-by-char
- 'peekBuffer' streams don't have the issue above, but still have to have some kind of extra state management to track delimiter
Still, original Stream methods could be improved? idk if users care about internals here, only that String is the end result.
i.e.master...mcspr:esp8266-Arduino:strings-webserver/pr9005 as a small experiment where we don't care about the type of Stream, and utilize String buffer as a window into delimited data
(which btw would extend to some other internal libs, not only webserver)
impl based onesp8266#9005, but for existing Arduino methods
impl based onesp8266#9005, but for existing Arduino methods
I was getting many
Reallocating large StringWarnings when decoding a long URL, and I found that the length of the string is already known before allocating, so it made sense, and it made a big difference.