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.

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

Remove enable arith boolean from Data / Field #535

Open
antoninbas opened this issue Jan 22, 2018 · 0 comments
Open

Remove enable arith boolean from Data / Field #535

antoninbas opened this issue Jan 22, 2018 · 0 comments
Assignees

Comments

@antoninbas
Copy link
Member

antoninbas commented Jan 22, 2018

This was meant as a performance enhancement: when parsing a field, we only create a bignum if this field is going to be used for arithmetic operations, based on the analysis of the json. This has been causing many issues over time (in particular with header stacks, for each we end up enabling arithmetic on every field for every header in the stack pretty much as soon as the stack is referenced in an expression). The performance gain is also dubious.

It seems that a better & simpler solution (if we want to avoid enabling arith on every field by default) would be to do lazy on-demand conversion from the byte representation to the bignum integer representation and vice-versa.

For example, we could have a mutable boost::optional<bignum> int_ and a mutable boost::optional<ByteContainer> bytes_ members for the Data class (and get rid of the Field class ?).

  1. When extracting a field, int_ would be set to none and bytes_ to the extracted value.
  2. When assigning:
void set(const Data &src) {
    this->int_ = src.int_;
    this->bytes_ = src.bytes_;
}
  1. When converting the value to an integral type:
  template<typename T, typename std::enable_if<std::is_integral<T>::value, int>::type = 0>
  T get() const {
    // lazy conversion
    if (this->int_ == boost::none) import_bytes();
    assert(this->int_ != boost::none);
    return int_.convert_to<typename std::remove_const<T>::type>();
  }
  1. When getting the byte representation (for table lookup or emitting...)
const ByteContainer &bytes() const {
    if (this->bytes_ == boost::none) export_bytes();
    assert(this->bytes_ != boost::none);
    return this->bytes_;
}
  1. For arithmetic
void add(const Data &src1, const Data &src2) {
    if (src1.int_ == boost::none) src1.export_bytes();
    if (src2.int_ == boost::none) src2.export_bytes();
    this->int_ = src1.int_ + src2.int_;
}
  1. etc

Note that we always have the following invariant: assert(this->int_ != boost::none || this->bytes_ != boost::none).

We would have to benchmark it on a complex P4 program (current approach vs proposed approach) but code modifications may not be too daunting. This new approach would be much less error-prone that the current one and I believe it has the potential of being faster despite the increased branching, given how conservative we have become with the enable arith method. Note how when copying one field to another we would not necessarily need to copy bignums or systematically export bytes like we do today.

@antoninbas antoninbas self-assigned this Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant