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.

Sign up for GitHub

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

Jump to bottom

Refactor output #1778

Merged
merged 4 commits into from
May 10, 2021
Merged

Refactor output #1778

merged 4 commits into from
May 10, 2021

Conversation

viktormalik
Copy link
Contributor

@viktormalik viktormalik commented Apr 8, 2021 •
edited
Loading

Following the discussion in #1705 (comment), I tried to do some refactoring of the Output class. Main changes in this PR are:

  • moved all methods doing conversion of values into strings into Output
  • removed code duplicated between TextOutput and JsonOutput by moving it into the base Output class
  • Output now defines an interface that all output classes must define
  • at the same time, Output implements some methods that are likely to behave the same between all output formatters
  • one of the effects is that JSON printing now handles nested structs

There's surely room for more improvement, so feel free to suggest more changes that could be done here.

Checklist
  • Language changes are updated in docs/reference_guide.md
  • User-visible and non-trivial changes updated in CHANGELOG.md
  • The new behaviour is covered by tests

@fbs
Copy link
Member

fbs commented Apr 13, 2021

need to stare at it a bit more but looks good

@fbs
Copy link
Member

fbs commented Apr 21, 2021

looks good.

@andreasgerstmayr, are you interested in giving this a go? I think you're the most active user of this.

@andreasgerstmayr
Copy link
Contributor

looks good, I'll try it out next week.

@andreasgerstmayr
Copy link
Contributor

I tested it today, works fine!

One thing I noticed is that now, instead of sending the output direct to the output stream out_, it's first buffering it in std::vector<std::string> and then joining into a new string (looking at Output::map_to_str). It's certainly more elegant, but requires more memory. str_join() keeps appending to a std::string - afaics that used to be very inefficient (allocating new strings every time), but I also read that recent C++ versions optimize that, so it shouldn't be an issue anymore?

fwiw, I didn't notice an impact for my application. I don't know if this might become an issue with very big maps, what do you think @fbs? If we continue on this route (building data structures before printing), we could also consider vendoring a JSON library in the future?

@viktormalik
Copy link
Contributor Author

Thanks for the review @andreasgerstmayr!

Good point about the string efficiency. I don't expect bpftrace users to create maps so huge that this would actually matter, so I prefer the elegant solution rather than a highly optimized one. But if someone thinks that we should address the issue, it shouldn't be a problem to re-implement this. I'm not sure how efficient modern compilers are in terms of string concatenation, but we could probably pre-reserve enough memory in str_join to prevent repetitive re-allocation. Or we can print to out_ directly, which would be even more efficient (not requiring the buffer at all).

@andreasgerstmayr
Copy link
Contributor

I'm fine with both ways, I'll leave it to @fbs if the PR should be updated regarding the string allocations or not.

@fbs
Copy link
Member

fbs commented May 4, 2021

Would be nice to have but not blocking. Lets just get this in :)

@fbs fbs added this to the 0.13.0 milestone May 4, 2021
@fbs
Copy link
Member

fbs commented May 9, 2021

@viktormalik can you fix the changelog iissue?

@viktormalik
Move reduce_value, min_value, max_value to Utils
00522d0 
Previously static methods of the BPFtrace class can be used from other
classes this way.
@viktormalik
Refactor conversion of value to string
aff9c04 
- move the method map_value_to_str from BPFtrace to Output
- add handling of tuples
- this allows to simplify printing of values from the JsonOutput class
- JSON printing of tuples containing aggregate types (structs or arrays)
  is now working
@viktormalik
Output: refactor print_map*
49eabc9 
There was a lot of code duplicated between TextOutput and JsonOutput.
This commit moves the common code for printing maps into the parent
Output class and leaves only the different parts to the extending
classes.
@viktormalik
Update CHANGELOG (JSON output improvement)
40db251
@viktormalik
Copy link
Contributor Author

Fixed, ready for rebase.

Should I open an issue for the string allocation optmization?

@fbs fbs merged commit 470807c into bpftrace:master May 10, 2021
@viktormalik viktormalik deleted the refactor-output branch May 11, 2021 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.
3 participants
@viktormalik @fbs @andreasgerstmayr