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

complete refactoring of Grid class #2765

Open
clyne opened this issue Jun 30, 2021 · 2 comments
Open

complete refactoring of Grid class #2765

clyne opened this issue Jun 30, 2021 · 2 comments
Labels
High Refactor Needed

Comments

@clyne
Copy link
Collaborator

clyne commented Jun 30, 2021

The Grid class needs some minor refactoring to complete the conversion from the use of std::vector to std::array for the representation of fixed-size, arrays of length 3. In particular, a number of public getters on the grid class still return std::vector instead of std::array. This results in unnecessary, and sometimes expensive, conversions between the types.
@shaomeng
Copy link
Contributor

shaomeng commented Jul 1, 2021

Well, in the proposed refactoring work, one should also move all the function implementations to the .cpp file, instead of leaving them in the .h file. That's because:

  1. the current headers are often more than 1000 lines long, kinda ridiculous.
  2. those function implementations in the header will be compiled every time the header is included, which means compiled hundreds of times throughout VAPOR, greatly increasing the compilation time and binary size.
  3. separation between headers and implementations complies with the coding convention.

@shaomeng shaomeng mentioned this issue Jul 1, 2021
Merged
clyne added a commit that referenced this issue Jul 6, 2021
@clyne
Partially addresses #2765. Changes internal representation of Grid::_…
2087772 
…dims

to match new API, which results in a ~10% improvement in performance.
@clyne clyne added the High label Jul 6, 2021
clyne added a commit that referenced this issue Jul 7, 2021
@clyne @shaomeng
Partially addresses Issue 2765 - complete refactoring of Grid class (#…
469fe04 
…2775)

* Partially addresses #2765. Changes internal representation of Grid::_dims
to match new API, which results in a ~10% improvement in performance.

* clang-format pre-push hook

* Fixed regression in last commit

* Update lib/vdc/Grid.cpp

Co-authored-by: Samuel Li <shaomeng@users.noreply.github.com>

* clang-format

Co-authored-by: Samuel Li <shaomeng@users.noreply.github.com>
@clyne clyne mentioned this issue Dec 21, 2021
Merged
@clyne
Copy link
Collaborator Author

clyne commented Mar 15, 2022

This has nearly been completed as of 3.6. I think we should defer further action until we have a more extensive testing framework, enabled by the upcoming Python API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
High Refactor Needed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@shaomeng @clyne