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

[Feature Request] Subview pattern. #648

Closed
kyungjoo-kim opened this issue Feb 17, 2017 · 8 comments
Closed

[Feature Request] Subview pattern. #648

kyungjoo-kim opened this issue Feb 17, 2017 · 8 comments
Assignees
Labels
Feature Request Create new capability; will potentially require voting
Milestone

Comments

@kyungjoo-kim
Copy link
Contributor

kyungjoo-kim commented Feb 17, 2017

Hello,

I use subview in many places. Subview mechanism is good for keeping consistent API interface. However, it is expensive when subview is used inside of a loop. For instance

// rank 2 array
auto multiple_vector_a = View<double**>("a", num_vectors, vector_length);
for (int j=0;j<num_vectors;++j) {
  // rank 1 array, which is a vector
  auto a = subview(multiple_vector_a, i, ALL()); // this is still expensive
  // this function requires rank 1 array
  doSomethingOnVector(a);
}

I believe that many people have similar situation to this. In this situation, subview adds quite overhead as it creates for each iteration. However, the subview pattern is fixed and we can take advantage of it. What I propose is

// rank 2 array
auto multiple_vector_a = View<double**>("a", num_vectors, vector_length);

// this subview creates subview patterns and compute offsets.
// here all view meta data can be constructed without a data pointer. 
// so now null data pointer is returned. 
auto a = subview(multiple_vector_a, FIXED, ALL()); 
for (int j=0;j<num_vectors;++j) {
  // since this already went thgouth subview, it should be unmanaged view
  // and changing the data pointer won't matters
  a.set_data(&multiple_vector_a(i,0,0));
  // this function requires rank 1 array
  doSomethingOnVector(a);
}

Or to ensure subview meta data is consistent, something as follows may be considered:

// for debug mode, validate shape of a and subview structure given 
// for release mode, this only swap the pointer and ignore subview shape arguments 
attach_subview(a, multiple_vector_a, i, ALL);

Kokkos view provides convenience but many people including me may consider to strip out the view when performance matters. This subview mechanism can reduce overhead and still provide luxury of view.

@hcedwar
Copy link
Contributor

hcedwar commented Feb 17, 2017

Consider something like

decltype( subview(a,i,ALL,ALL) ) aa ;
for ( ... ) {
  assign_subview( aa , a , i , ALL , ALL );

@hcedwar hcedwar added the Feature Request Create new capability; will potentially require voting label Feb 17, 2017
@hcedwar hcedwar added this to the Backlog milestone Feb 17, 2017
@kyungjoo-kim
Copy link
Contributor Author

kyungjoo-kim commented Feb 17, 2017

I am fine as long as I can reduce subview overehad to a simple pointer swapping.

So, the "decltype" is completely portable for everywhere, right ?

@hcedwar
Copy link
Contributor

hcedwar commented Feb 22, 2017

Dimensions of subview remain fixed, only the underlying address changes at each iteration.

@hcedwar hcedwar modified the milestones: 2017-April-end, Backlog Feb 22, 2017
@crtrott crtrott modified the milestones: 2017-April-end, 2017-June-end Apr 26, 2017
@hcedwar hcedwar modified the milestones: Backlog, 2017-June-end Apr 26, 2017
@kyungjoo-kim
Copy link
Contributor Author

kyungjoo-kim commented Aug 24, 2017

@hcedwar

Just regular ping to see how this is going on. In Andrew's implementation of line smoother, this capability would improve performance a bit.

@ambrad
Copy link

ambrad commented Aug 25, 2017

In the line smoother solve phase, which is O(nnz) and so very sensitive to overhead, using raw pointers instead of subview gives a speedup of 1.2 to 1.5 for block sizes of interest. The solve phase is often the most time consuming by an order of magnitude or more.

@hcedwar hcedwar modified the milestones: 2017-September (mid), Backlog Aug 28, 2017
@hcedwar hcedwar modified the milestones: 2017 September, 2017 October Sep 20, 2017
@hcedwar hcedwar self-assigned this Sep 20, 2017
@hcedwar hcedwar modified the milestones: 2017 October, 2017 September Sep 20, 2017
hcedwar added a commit that referenced this issue Sep 20, 2017
and assigns the data pointer.

Proper use case is to update subviews. For example:

  auto sub_a = subview(a,0,ALL,ALL,ALL);
  for ( int i = 0 ; i < a.dimension(0) ; ++i ) {
    sub_a.assign_data( & a(i,0,0,0) ); // now compute on sub_a

    // previously had to do:
    // auto sub_a = subview(a,i,ALL,ALL,ALL);
    // which has large overhead.
  }
@hcedwar
Copy link
Contributor

hcedwar commented Sep 20, 2017

FYI: @ambrad @kyungjoo-kim , this is being tested now to go into the next promotion.

@ambrad
Copy link

ambrad commented Sep 20, 2017

Nice. @mhoemmen and @mndevec are probably interested in this, as well.

@mhoemmen
Copy link
Contributor

@ambrad It could be nice to have this feature for sparse matrices, though I think it's most critical for (small dense) block sparse matrices.

hcedwar added a commit that referenced this issue Sep 21, 2017
@crtrott crtrott closed this as completed Oct 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Create new capability; will potentially require voting
Projects
None yet
Development

No branches or pull requests

5 participants