Repository navigation
(perf) std::list => std::vector - #99
mathieucarbou wants to merge 1 commit into
Conversation
|
@vortigont : would you be able to look at this pr please ? |
5b36a91 to
96bbef2
Compare
There was a problem hiding this comment.
Will speed up request processing by avoiding array reallocations at the expense of taking a little more memory because adding interesting headers is done for each entering request
There was a problem hiding this comment.
Will decrease memory usage using array at the expense of slower insertion and removal which is acceptable for rewrites and handlers which are not often added or removed
There was a problem hiding this comment.
it depends on realization actually. vector usually preallocate certain size on initialization, so in case of rewrites it might take mem even when no rules are added at all. So I'm not sure that this change makes much sense considering mem allocation. Access pattern could be another concern but since container keeps pointers to other objects and those are accessed/added sequentially and removed randomly I was thinking that linked list could be a good compromise choice here.
96bbef2 to
6fcdce1
Compare
|
Hey @mathieucarbou! |
a5432f5 to
46fef12
Compare
46fef12 to
9798d49
Compare
mathieucarbou
changed the title
|
Hey @mathieucarbou! Pls see my comments inline. Will try to suggest some ideas on weekend, still busy with main job :( |
no problem. i am still testing the middleware PR. |
There was a problem hiding this comment.
This looks as a design gap for me.
Comparing a const char* pointers is not the same as comparing the literals it points too. There could be false negatives where two different pointers points to two identical literals but in different mem places, i.e. one comes from a const char* in ROM and another via some UI's acquired var.
Here is a nice place to use std::string_view but it needs c++17. Otherwise have to use a String or std::string to do actual string comparison here if needed or simple std::strcmp if no wide unicode magic required.
There was a problem hiding this comment.
this is the most ugly part of the legacy code as for me. Comparing a pointer to an object with a list of unique pointers is so wrong. If it's a unique pointer then it can't share lifetime of an object with any other pointer by design. It's a hack due to keeping compat method allowing to add handlers via pointer but having and unique_ptr container.
I do not have an easy way to make it right and not to change the API. Maybe postpone the change for some breaking the API major release.
I would rather expanded the AsyncWebHandler class with some kind of id member or make a container of std::pair with id:AsyncWebHandler* where id is autogenerated on adding the handler. It's just one of the possible options to consider.
There was a problem hiding this comment.
it depends on realization actually. vector usually preallocate certain size on initialization, so in case of rewrites it might take mem even when no rules are added at all. So I'm not sure that this change makes much sense considering mem allocation. Access pattern could be another concern but since container keeps pointers to other objects and those are accessed/added sequentially and removed randomly I was thinking that linked list could be a good compromise choice here.
|
@vortigont : ok thanks ! I will close the PR then :-) let's revisit that later. Thanks! |
|
maybe split it into different smaller tasks would be easier to implement/discuss |
What do you have in mind ? Do you mean have a project board where we could list some tasks to improve the project? |
|
I've created a kanban board: https://github.com/mathieucarbou/ESPAsyncWebServer/projects?query=is%3Aopen |
2 participants
No description provided.