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

Code question: style vs performance #395

Closed
phkahler opened this issue Apr 19, 2019 · 2 comments
Closed

Code question: style vs performance #395

phkahler opened this issue Apr 19, 2019 · 2 comments
Labels
question

Comments

@phkahler
Copy link
Member

When there is a possible small optimization, I'm guessing the preference is not to do it if it has any impact on readability or clarity. Correct? I noticed this extra temporary SSurface in SSurface::SplitInHalf() and wondered if there is any measurable performance difference just using sa 3 as a temporary rather than creating a whole new surface for cubics. So instead of this:
`

    case 3: {
        SSurface st;
        st.degm = degm; st.degn = degn;

        sa->CopyRowOrCol (byU, 0, this, 0);
        sb->CopyRowOrCol (byU, 3, this, 3);

        sa->BlendRowOrCol(byU, 1, this, 0, this, 1);
        sb->BlendRowOrCol(byU, 2, this, 2, this, 3);
        st. BlendRowOrCol(byU, 0, this, 1, this, 2); // scratch var

        sa->BlendRowOrCol(byU, 2, sa,   1, &st,  0);
        sb->BlendRowOrCol(byU, 1, sb,   2, &st,  0);

        sa->BlendRowOrCol(byU, 3, sa,   2, sb,   1);
        sb->BlendRowOrCol(byU, 0, sa,   2, sb,   1);
        break;

We could do this:

    case 3: {
        sa->CopyRowOrCol (byU, 0, this, 0);
        sb->CopyRowOrCol (byU, 3, this, 3);

        sa->BlendRowOrCol(byU, 1, this, 0, this, 1);
        sb->BlendRowOrCol(byU, 2, this, 2, this, 3);
        sa->BlendRowOrCol(byU, 3, this, 1, this, 2); // use as scratch var

        sa->BlendRowOrCol(byU, 2, sa,   1, sa,  3);
        sb->BlendRowOrCol(byU, 1, sb,   2, sa,  3);

        sa->BlendRowOrCol(byU, 3, sa,   2, sb,   1);  // no longer scratch var
        sb->BlendRowOrCol(byU, 0, sa,   2, sb,   1);
        break;

`

It seems like a possible win, but I doubt it's anywhere near a big deal in real world performance in this case. The temporary will need to be given stack space even for non-cubics, but there is no constructor/destructor overhead per SS style right?

Which way should we fall on things like this? It bothers me because is some kinds of code this would be a big deal, and I don't have a way to measure here.
@whitequark
Copy link
Contributor

The temporary will need to be given stack space even for non-cubics, but there is no constructor/destructor overhead per SS style right?

Stack space doesn't matter, it's extremely cheap. Indeed there is no constructor/destructor overhead in most cases, and at most it's zeroing some data.

Which way should we fall on things like this? It bothers me because is some kinds of code this would be a big deal, and I don't have a way to measure here.

In general, the code in SolveSpace is often very tricky and not exhaustively tested at all. So I always err on the side of clarity.

In this case, do we win anything at all? Are we even reusing the heap allocation?

@phkahler
Copy link
Member Author

In this case, do we win anything at all? Are we even reusing the heap allocation?

There is no heap allocation here. I was just thinking this function is not needed because there is no reason to split a surface in half since we have arbitrary trim curve. Splitting the surface would just require reparameterizing all UVs for no reason. So I looked and this function appears to not be called at all. But the question was more about preferences. I think I understand the preferences.

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

No branches or pull requests
2 participants
@whitequark @phkahler