Deserialization cleanup #2121

crsib · 2021-11-11T20:15:29Z

The idea behind the PR:

Have a class, that is fast to construct and allows to handle integer, floating-point, and string_view types of data.
Replace all the redundant casts to UTF16, use string_view to represent tag and attribute names
Using the updated XMLTagHandler create an adapter, that allows deserializing the project directly from the binary stream.

The infamous project I mentioned in #2087 loads in:

11 seconds on 3.1.0,
5 seconds on 3.1.1,
270 msecs with this build.

Additionally, I have a project from @Penikov, which has a 420 Mb project blob. For this case, I observe:

95 seconds on 3.1.0 (and a whooping peak around 4 Gb of RAM, while only 1.4 is needed!),
45 seconds on 3.1.1,
~2.5 seconds with this build.

The dark side of this PR:

All the XML (de)serialization is touched. This includes exporting for VST2, Equalization, and importing legacy aup files.
There is a large number of files changed, most of them related to the project loading though.

I signed CLA
The title of the pull request describes an issue it addresses
If changes are extensive, then there is a sequence of easily reviewable commits
Each commit's message describes its purpose and effects
There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

Each commit compiles and runs on my machine without known undesirable changes of behavior

Paul-Licameli · 2021-11-16T11:33:19Z

libraries/lib-xml/XMLTagHandler.cpp


-   bool result = HandleXMLTag(UTF8CTOWX(tag), out_attrs.get());
+   bool result = HandleXMLTag(tag, mCurrentTagAttributes);


By eliminating UTF8CTOWX, are you requring that tag strings to be canonicalized into utf-8 already, or requiring them just to be ASCII strings? They are all ASCII in practice (I just had to review them all). It is worth a comment.

You know I like to say things with type disctinctions where I can. If we were using C++20, I would use std::u8string_view instead of std::string_view. Not having C++20 and char8_t, what can we do? Maybe at least make a different type alias to make the disctinction in function declarations, thought we won't have type checking by the compiler.

I don't actually think that char8_t implies any checks on encoding. However, we use Expat, and Expat seems to convert the output to UTF8 internally. So specifically for this case, I imply UTF8, and that is what originally implied.

The UTF8 allows to safely compare to ASCII strings given that all the symbols are from the first half of the table. This covers Audacity needs for 100% when working with tags and names. For attribute values - we explicitly convert from UTF8 to wxString where needed.

Certainly it's safe when a tag is ASCII, and all of them are. But I'm asking, are we requiring them to be ASCII now that you remove UTF3CTOWX ?

You may miss my point about char8_t -- it is that, where a string or string_view is required to be utf8 encoding, it would be good to say that with one type, and where required to be ASCII, to say that without another type, and it would be best if the types do not implicitly interconvert, so the compiler will help us catch misuses.

are we requiring them to be ASCII

We can't even do it, because Expat will always output UTF8 encoded string.

where a string or string_view is required to be utf8 encoding

The safest assumption is that all std::string are UTF8 encoded to be fair.

Paul-Licameli · 2021-11-16T11:33:19Z

libraries/lib-xml/XMLTagHandler.cpp


-   bool result = HandleXMLTag(UTF8CTOWX(tag), out_attrs.get());
+   bool result = HandleXMLTag(tag, mCurrentTagAttributes);


By eliminating UTF8CTOWX, are you requring that tag strings to be canonicalized into utf-8 already, or requiring them just to be ASCII strings? They are all ASCII in practice (I just had to review them all). It is worth a comment.

You know I like to say things with type disctinctions where I can. If we were using C++20, I would use std::u8string_view instead of std::string_view. Not having C++20 and char8_t, what can we do? Maybe at least make a different type alias to make the disctinction in function declarations, thought we won't have type checking by the compiler.

Paul-Licameli · 2021-11-16T12:03:11Z

I pushed a suggested commit, please review that!

Paul-Licameli · 2021-11-16T12:06:16Z

libraries/lib-string-utils/3party/fast_float.h

@@ -0,0 +1,2947 @@
+// fast_float by Daniel Lemire


I'm taking this whole file on trust. Are there any differences from the original I should know? As there were with ToChars?

This time it is just the latest release of the library!

Paul-Licameli · 2021-11-16T12:08:37Z

libraries/lib-string-utils/FromChars.h

+//! Result of the conversion, similar to std::from_chars_result
+struct STRING_UTILS_API FromCharsResult final
+{
+   const char* ptr; //! A pointer to the first character no matching the the pattern


"not matching the pattern"

libraries/lib-string-utils/FromChars.h

libraries/lib-string-utils/FromChars.cpp

crsib · 2021-11-16T12:32:07Z

libraries/lib-xml/XMLMethodRegistry.h

   TagTable mTagTable;
+   std::forward_list<std::string> mTags;


Probably std::deque? It can be more efficient on some platforms

I thought deque deals with big pages. Existing ProjectFileIORegistry (alias XMLMethodRegistry) registrations are maybe too few to justify it, and they happen only at startup.

Whereas, the lookups into the map may be many, and we want all the savings we can find during deserialization.

The lookups may not really be many, yet, but XMLMethodRegistry may be reused for many more fields later.

libraries/lib-string-utils/FromChars.cpp

crsib · 2021-11-16T12:35:33Z

I pushed a suggested commit, please review that!

I've left that out to see if I can see a measurable difference in performance and I've observed none. Still, that commit is a welcome addition, thank you!

libraries/lib-string-utils/FromChars.cpp

Paul-Licameli

Overflow detection is insufficient

libraries/lib-string-utils/FromChars.cpp

libraries/lib-xml/XMLAttributeValueView.h

Paul-Licameli · 2021-11-18T02:38:09Z

src/ProjectFileIO.cpp

+            FromChars(
+               valueString.data(), valueString.data() + valueString.length(),
+               value)
+               .ec;


Is it OK to ignore .ptr in the FromCharsResult? (This is the only place you do ignore it)

Paul-Licameli · 2021-11-18T04:07:26Z

Something is definitly wrong with autosave now. Do these steps:

Run Audacity; generate noise; force-quit Audacity (such as with the debugger)
Restart Audacity; recover files; you should get the noise back.
Force-quit again; restart again; recover again.

Expected result: the second recovery works.
What I observe: varying crash-y symptoms suggestive of a memory overwrite.

I think you must recover twice to get the bug, and I think it happens only starting at the commit "Use the fast stream reader (still, reading from memory)". Of course it may be hard to be sure, when there is a memory overwrite.

src/ProjectSerializer.cpp

Paul-Licameli · 2021-11-18T05:43:55Z

src/ProjectSerializer.cpp

+         }
+      }
+
+      mStringsCache.clear();


mStringsCache is a deque, I assume because you need non-relocation of the strings that you cache in it (so that you can make string_views later).

But could repeated clearing of the deque and then re-allocation in it doing some wasteful reallocations? Are all pages freed when you clear()? Standard says std::vector::clear() does not change capacity, but I don't see the same stipulated for deque.

Maybe you can keep one dummy string in it, and resize to 1 instead of clearing, if it matters.

You really don't like deque ;-)

Usually, the problem with deque is considered to be different: the memory costs just for one item are very high because deque allocates in large chunks. std::sting is usually around 24-32 bytes (why, libc++?. No, really, why 32 bytes?). On Clang deque allocates no less the 4k, on GCC - 512 bytes, which surely fits quite a bit of strings in. Even horrible MSVC implementation is not a performance concern here. We have worse problems to be solved first, for example, Sequence::HandleXMLTag is currently the #1 issue, because it never estimates the resulting blocks count, so mBlock.push_back(wb); is consuming most of the time right now. Still, we have a 40x speed up, and that problem can be easily addressed in a separate, much smaller, and easier to review PR.

In case we ever end up having deque to be a performance problem here - the fix will be as easy as a custom allocator. And trust me - you have to write them a lot when you are fighting for the performance :-)

Custom allocator. I was wondering if you would find use for std::pmr::string and some other allocator for the character data.

Paul-Licameli · 2021-11-18T05:45:27Z

src/ProjectSerializer.cpp

+      { return static_cast<std::make_unsigned_t<BaseCharType>>(c) < 0x7f; });
+
+   if (isAscii)
+      return std::string(begin, end);


This looks correct, but I don't understand what economy it gives you. Still there is one std::string construction. Are you sure there are in fact even more allocations in the non-ascii path?

Around 15-20 percent speedup. C++ standard library fails horribly when it comes to working with strings and locales (nothing new, really. It was this way since the STL was created). Another approach would be to use alternative converters. It will be a bit slower in general, but much faster if there are lots of non ASCII labels in the project.

crsib · 2021-11-18T09:11:38Z

Something is definitly wrong with autosave now. Do these steps:

There was an issue with recovering a project, that never was saved. I do not observe the issue you described though

Paul-Licameli · 2021-11-18T10:20:53Z

Something is definitly wrong with autosave now. Do these steps:

There was an issue with recovering a project, that never was saved.

Which issue? If it happens, it surely is an issue to fix. The unsaved data might be a long recording.

I do not observe the issue you described though

I know I saw this problem, and I find it reliably in my macOS debug build of this branch, though the exact symptoms are various. I'm not satisfied until there is more understanding.

Paul-Licameli · 2021-11-18T10:33:22Z

Something is definitly wrong with autosave now. Do these steps:

There was an issue with recovering a project, that never was saved.

Which issue? If it happens, it surely is an issue to fix. The unsaved data might be a long recording.

I do not observe the issue you described though

I know I saw this problem, and I find it reliably in my macOS debug build of this branch, though the exact symptoms are various. I'm not satisfied until there is more understanding.

Your latest change to LoadProject may have made the problem go away, so it seems with a few tries, although I don't know why.

crsib · 2021-11-18T10:38:53Z

I do not observe any problems with the latest build on macOS, not in release or debug builds. I think there can be an issue in ShowError

Paul-Licameli · 2021-11-18T11:11:10Z

class XMLUtf8BufferWriter became unused at "Decode BXML directly, without intermediate stage"

So should the class be deleted? Sad all that hard work obsoleted so soon.

Paul-Licameli · 2021-11-18T11:13:24Z

I just need to step through ProjectFileIO::LoadProject to be sure I understand all the changes in there, and I will approve if I think nothing is wrong, although the unexplained random problems I saw give me a bit of doubt that some problem was missed.

crsib · 2021-11-18T11:19:49Z

So should the class be deleted? Sad all that hard work obsoleted so soon.

To be fair, I think it's XMLStringWriter that will go away at some point, and XMLWriter interface will be adapted to match XMLUtf8BufferWriter. There is still room for performance gains in serialization, that can be achieved by removing const wxString &name from the writer interface :-) And I expect much easier PR for that!

Paul-Licameli

Please squash the history a little, and be sure the long PR comment is copied when the Merge button gives you the entry window for it.

Let's go!

I am glad to see the known silliness of the in-memory tempoary XML corrected.

For floats - excellent MIT licensed implementation by prof. D. Lemire is used. For integers - some template magic is used for universal, overflow checking implementation.

XMLAttributeValueView is a view class that holds possible attribute values. XMLAttributeValueView does not take ownership for the string values, so it should never be stored.

…ings

crsib · 2021-11-18T12:13:04Z

Well, that was epic!

crsib requested review from Paul-Licameli and vsverchinsky Nov 11, 2021

crsib assigned crsib and Penikov Nov 11, 2021

crsib added this to In progress in Sprint 8 - 3.1 Stabilisation via automation Nov 11, 2021

crsib force-pushed the deserialization_cleanup branch from f59a12a to 6ac095e Compare Nov 11, 2021

Penikov marked this pull request as ready for review Nov 12, 2021

crsib marked this pull request as draft Nov 12, 2021

crsib mentioned this pull request Nov 12, 2021

Audacity 3.1.1 will crash while opening projects containing very small double values #2127

Closed

crsib force-pushed the deserialization_cleanup branch from 6ac095e to fddf234 Compare Nov 12, 2021

crsib changed the base branch from release-3.1.1 to release-3.1.2 Nov 12, 2021

crsib force-pushed the deserialization_cleanup branch from 68701f9 to 05c2aba Compare Nov 15, 2021

crsib marked this pull request as ready for review Nov 15, 2021