Improve memory track visualization with the stacked graph

[vickyliu-go4it]

With this change, we change the MemoryTrack to visualize data as a stacked graph.

We also plan to refactor the current graph related tracks according to the following diagram.

The detailed plan to refactor the current graph related tracks and add / update memory tacks and pagefault tracks is:

• STEP 1. Rename current GraphTrack as VariableTrack
• STEP 2. Add the new GraphTrack
• STEP 3. Add the AnnotationTrack
• STEP 4. Add the MemoryTrack for system memory usage
• STEP 5. Add the MemoryTrack for process & cgroup memory usage
• STEP 6. Add the PagefaultTrack
• STEP 7. Change the VariableTrack to inherit from the new GraphTrack

And this PR contains the changes from STEP 1 to STEP 4.

Memory track after the change:

Updated class diagram:

Bug: http://b/185108718
Test: Run orbit and take a capture with enabling memory tracing.

[beckerhe]

Very nice, Vicky. I added some comments. 🙂

[antonrohr]

Hey, sorry, this PR takes me forever to review. I am not completely done yet, but here is the first batch of comments. I really like this PR and in addition to beckerhe's comments, I have some more ideas to make it even better.

[antonrohr]

I am now done with the review :)

[beckerhe]

Looks even better now. Thanks for all the work Vicky and sorry for being so late with the review. I added some more comments. 🙈

[beckerhe]

This function should probably be private

[vickyliu-go4it]

Thanks for pointing out this. This might be called by the MemoryTrack when we set the value upper bound and lower bound.

[beckerhe]

Hmm. In that case I would suggest to not put min_ and max_ into this class. If these values can be changed independently from the time series then they don't share any invariance and shouldn't be in the same class.

[vickyliu-go4it]

Thanks for reviewing this PR! Also sorry for making this so difficult to review 🙈 I should split it into smaller ones!

Also could you give some suggestion on the collapsed version? Should we keep drawing the while label when the graph track is collapsed?

[antonrohr]

Thank you for addressing my earlier comment and please take the following as a personal preference. I did a bad job requesting a split up to increase readability, because I did not go into more detail on what I actually mean.

Beforehand it was this:

absl::StrAppend(&text, delimiter,
                series_names[i].empty() ? "" : absl::StrFormat("%s: ", series_names[i]),
                value_decimal_digits_.has_value()
                    ? absl::StrFormat("%.*f", value_decimal_digits_.value(), values[i])
                    : std::to_string(values[i]),
                label_unit_);

I would argue its easier readable and understandable if it has some extra variable names, so I dont need to keep partial result in my head. I dont know if what Im saying makes sense. This is what I mean:

std::string formatted_value =
    value_decimal_digits.has_value()
        ? absl::StrFormat("%.*f", value_decimal_digits.value(), values[i])
        : std::to_string(values[i]);
std::string formatted_name =
    series_names[i].empty() ? "" : absl::StrFormat("%s: ", series_names[i]);

absl::StrAppend(&text, delimiter, formatted_name, formatted_value, label_unit);

[vickyliu-go4it]

Changed accordingly! Also sorry for misunderstanding your suggestion previously. I like your idea of adding more variable with readable names here! Thanks!

[beckerhe]

Thanks Vicky. I added some more minor comments, but in general I think that's good to go!

[antonrohr]

I am not done with the next review round. I will look at it again on monday

[antonrohr]

Thank you a lot for adding my requests :)
Only one more small nit about method name cases.