[Impeller] Switched Command::label to LightweightString

[gaaclarke]

While profiling gallery it was noted that 160 MB/s was getting allocated from Commands. This remove 8 bytes per Command and the string duplication for each command. For example, 10 bytes per "TextFrame" command.

So, that works out to about 7 MB / s. It's not a huge savings but it's just wasted memory that can exacerbate memory fragmentation.

There isn't a good way to force string literals be used in C++ for instance variables that I could figure out.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[jonahwilliams]

Some of this information (blur size, texture name, et cetera) is very useful for debugging. But on the other hand, is completely useless to record if no one is looking at the profiles.

Can we retain this somehow on development and/or profile builds? Do we need to set the command label at all in release builds?

[jonahwilliams]

Like it would it be reasonable to leave the command.h declaration as a std::string, but then have a macro to set the command label like

Command cmd;
MAGIC_MACRO_DEBUG_LABEL(cmd, "Some string with runtime values");

Where that macro no-ops in certain build configs?

[jonahwilliams]

Or I think for Vulkan specifically, we'll never see these logs if validation layers aren't enabled.

[gaaclarke]

Yea, I've been wrestling with this PR in my mind. I have one more idea that i'm trying out, trying to timebox it since it isn't the hugest change and I want to get around to pooling host buffers. I think I can make the labels just 8 bytes and avoid the copies if they are string literals, making it smaller and safer than what I have here right now.

[gaaclarke]

@jonahwilliams please give this a look. This is the best I could come up with. It's slightly bigger than a raw pointer but eliminates copying of string literals, which is usually how Command::label is used and is definitely the case for the Commands from text_contents.cc.

I was wary of conditional compilation since someone may want to look at this stuff in release builds and making the debug builds as close to the release builds as possible is preferable.

I can add tests for LightweightString if we are happy with this.

[jonahwilliams]

TBH I don't really feel like I have the C++ expertise to correctly review this implementation but I like the idea. I might name it differently though, its not that its lightweight its that it owns the underlying std::string to prevent copying?

[jason-simmons]

I would vote for conditional compilation.

Beyond the cost of string copying I was also noticing some costs of building labels showing up in profiles (e.g. the SPrintF call in RenderPass::SetLabel).

The debug labels for Impeller objects are not useful outside of a development environment. Having a consistent way to disable labels for profile/release builds would make it simple to save unnecessary work.

[gaaclarke]

The problem with conditional compilation is that debugging memory usage is easier with debug builds. If builds start behaving differently it's harder to track usage.

@jason-simmons in a previous commit that i wiped out I removed the sprintf calls for string literals in some cases. I could bring that back if we want. The cost is a bigger binary.

This is an improvement over what we have today but seems a bit contentious about what the best course is and is small potatoes, so I'm going to put a pin in it.

[chinmaygarde]

Stashing this for now but we may need to revisit this in the future. I am for something like this but it may not be the right time yet. I'm putting a reference to this in the icebox so we can find it again later.

[gaaclarke]

The size of Command came up again in Jonah's profiling earlier this week. We may want to revisit this.

[gaaclarke]

Looks like jonah did something to address this here: #44274