Optimize hot path in OutputCellIterator

[skyline75489]

Summary of the Pull Request

References

PR Checklist

• Supports [Performance] vtebench tracking issue #10563
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
• Schema updated.
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

[skyline75489]

This has been sitting in my local repo for so long, might as well publish it to see if people like it. This is the OutputCellIterator version of #10621. Similar approach but this one is less impactful.

OpenConsole.exe (in ConPTY mode)

Before:

After:

Time needed for catting enwiki8 dropped from 16s to 15s. Before #10621, it takes about 21s.

I think after PGO this would be more impactful, because it clearly separates the hot path from the cold path.

[skyline75489]

@lhecker this is the OutpuCellIterator work that I mentioned before. I think it's still not ideal without the TextBuffer rewrite work, as you can see in previous comments. But it's better than nothing, I guess? I'll just leave the decision to you and @miniksa .

[skyline75489]

This branch here needs to be highly compact & optimized to avoid potential "I-Cache spill" (similar to the one in #10621), so I only handle the condition when _currentView.DbcsAttr().IsSingle()

[skyline75489]

Again, inspired by #10621, use UpdateText instead of calling expensive constructors.

[skyline75489]

Wait, _declspec works the same as __declspec?... I need this to be not-inlined to reduce the instruction count.

[lhecker]

To be honest I'd personally prefer if we replaced OutputCellIterator entirely in our hot paths.
It's exists as our data model is complex and requires a complex iterator to easily navigate. This shouldn't mean though that the design of our data model is an immutable fact and not just a coincidence of the way it was built over the years. We can restructure it to allow more efficient iteration.
Furthermore the _mode field in OutputCellIterator strongly indicates that this class should for instance be a template where _mode is a template parameter, or any other equivalent alternative. Because at the moment this class is sort of a "jack of all trades" (or the funnier German equivalent: an "egg-laying woolly milk sheep"), which heavily inhibits code clarity. Coincidentally it also inhibits performance.

[skyline75489]

Thanks @lhecker for the suggestion. Template sounds like a wonderful idea. I actually have the same concern as you and I won't be upset if people don't like this PR. Feel free do whatever may be suitable with this PR.

[DHowett]

In my FHL branch I've simply deleted OutputCellIterator and friends ;P

[skyline75489]

Glad OutputCellIterator won’t stay with us forever. I am closing this for now.