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

Some extra epee parsing speedups #5348

Merged
merged 1 commit into from Apr 6, 2019

Conversation

Projects
None yet
3 participants
@moneromooo-monero
Copy link
Contributor

moneromooo-monero commented Mar 25, 2019

No description provided.

@@ -54,31 +54,31 @@ namespace epee

portable_storage(){}
virtual ~portable_storage(){}
hsection open_section(const std::string& section_name, hsection hparent_section, bool create_if_notexist = false);
hsection open_section(const boost::string_ref& section_name, hsection hparent_section, bool create_if_notexist = false);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 25, 2019

Contributor

I recommend dropping the reference for the boost::string_ref parameters. With the exception of older ABIs, which should be the minority of users, this will use one additional register but internally is one less pointer indirection for the optimizer to handle.


namespace epee
{
namespace misc_utils
{
class backed_string_ref: public boost::string_ref

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 25, 2019

Contributor

Inheritance should not be used in this situation. Any function that takes boost::string_ref by value will "slice away" the "backing" portion of the std::string, leaving dangling references. The other "fix" is to ensure every function call uses boost::string_ref by const reference which adds minor perf differences due to another pointer indirection. I strongly discourage this "fix"; existing uses of boost::string_ref need to be updated and every usage of boost::string_ref/back_string_ref from every future commit needs to be manually inspected since the compiler will not generate errors with the invalid code.

@@ -124,14 +144,22 @@ namespace misc_utils
\\ Backslash character
*/
inline void match_string2(std::string::const_iterator& star_end_string, std::string::const_iterator buf_end, std::string& val)
inline void match_string2(std::string::const_iterator& star_end_string, std::string::const_iterator buf_end, backed_string_ref& sval)

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 25, 2019

Contributor

I don't think the interface should be changed at all, there's no benefit. When take_string() is called, if an allocation was skipped in here, it is then done externally. And the only case where take_string() is not called is for the name field in the epee storage class and that function is always creating a std::string from that parameter.

The problem with this interface is that sometimes the return-value owns the memory, other times not. I'm not sure that the backed_string_ref design makes this clear. It is easy to spot if you look at the implementation real quick, so I might be too pessimistic. But since there is never any savings of allocations in current usage, I don't see why the transition is helpful.

{
TRY_ENTRY();
CHECK_AND_ASSERT(psection, nullptr);
return &psection->m_entries[std::string(pentry_name.data(), pentry_name.size())];

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 25, 2019

Contributor

This is why the entire interface makes no sense. A backed_string_ref was created to remove an allocation inside of match_string2, yet the changes are such that it will always allocate a std::string right here or via take_string() in all other cases. There are no scenarios where the result of match_string2 does not allocate a std::string, making the interface harder to use with no current benefit.

{
TRY_ENTRY();
CHECK_AND_ASSERT(psection, nullptr);
auto it = psection->m_entries.find(pentry_name);
auto it = psection->m_entries.find(std::string(pentry_name.data(), pentry_name.size()));

This comment has been minimized.

Copy link
@vtnerd

vtnerd Mar 25, 2019

Contributor

I assume the mention of C++14 in the commit was due to this function call here. boost::container::map has the same interface as the STL version, and has the templated find method available for C++03 or newer compilers. Not sure if its worth transitioning to the library, but it is there.

This interface (without the templated find) also seems preferable only if the majority of the sources are not std::string. It seems the opposite is true, that ultimately all sources are std::string.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

moneromooo-monero commented Mar 25, 2019

The intent was that later, it might save a lot of allocs, and that now it only saves a few. Looks like it's not worth it, maybe only once we can actually save the bulk (and yes, as you say above, it's the find that causes the std::string allocs to be needed anyway).

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

moneromooo-monero commented Mar 25, 2019

Actually, the other commit's fine.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:esp branch from b831e30 to 59776a6 Mar 25, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

moneromooo-monero commented Apr 3, 2019

Are you OK with just the commit that's left ?

@vtnerd

vtnerd approved these changes Apr 4, 2019

@fluffypony
Copy link
Collaborator

fluffypony left a comment

Reviewed

@fluffypony fluffypony merged commit 59776a6 into monero-project:master Apr 6, 2019

3 of 10 checks passed

buildbot/monero-linux-armv7 Build done.
Details
buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
buildbot/monero-static-win64 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
buildbot/monero-linux-armv8 Build done.
Details
buildbot/monero-static-osx-10.13 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details

fluffypony added a commit that referenced this pull request Apr 6, 2019

Merge pull request #5348
59776a6 epee: some more minor JSON parsing speedup (moneromooo-monero)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.