Page MenuHomeWildfire Games

Avoid copying CChart m_Points following rP19027
Changes PlannedPublic

Authored by elexis on Jul 24 2019, 4:21 AM.

Details

Reviewers
wraitii
Summary

As reported by leper in rP19027, the points passed from JS to the C++ are constructed as C++ objects and then copied again by a copy-assignment. The copy may be avoided.
This patch also transforms the ENSURE into a JS_ReportError, which is what the rest of the code does.

Test Plan

Make sure that the move assignment can actually be performed here, that the source vector is discarded regardless.

Observe that ENSURE is actually doing the wrong thing in case a developer clicks on continue (continuing the next statement when the right next step would be to return from this function instead of continuing it.)

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8564
Build 14019: Vulcan BuildJenkins
Build 14018: arc lint + arc unit

Event Timeline

elexis created this revision.Jul 24 2019, 4:21 AM
elexis updated this revision to Diff 9090.Jul 24 2019, 4:23 AM

Missing #undef

historic_bruno added inline comments.
source/scriptinterface/ScriptConversions.h
37

Would it be too annoying to display the maximum size in the error message?

44

Same here, could we list which element?

70

And here

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/scripting/EngineScriptConversions.cpp
| 188| »   »   FAIL_VOID("Failed·to·get·Vector3D·constructor");
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If STMT is a macro then please configure it.

source/scriptinterface/ScriptConversions.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/212/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/scripting/EngineScriptConversions.cpp
| 188| »   »   FAIL_VOID("Failed·to·get·Vector3D·constructor");
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If STMT is a macro then please configure it.

source/scriptinterface/ScriptConversions.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/213/display/redirect

wraitii added inline comments.Jul 24 2019, 8:14 PM
source/gui/CChart.cpp
200

When doing this, the swap itself is O(1) but we call the destructor of all items previously in data.m_Points. However, I think CVector2D has a trivial destructor as it's 2 floats and not user provided, so this does nothing, and thus the whole operation is O(1) as expected.

One could also use data.m_Points.swap(pSeries->m_Series[i]), which guarantees constant complexity (we'll still run the destructors later, but that's again not relevant).

Now, the question is: is pSeries still used after? Would JS code try to read from it instead of just write to it? Would we call GetSettingPoint(this, "series") elsewhere?
As pSeries is a reference, it would be left in an undefined (though sane) state.

Checked for usage of the series attribute - it's not used anywhere else right now, so this would be safe to commit. Obviously, it wouldn't be correct if we ever needed to read series from the JS code though.

I'm not sure it's a good thing to commit this. It might break code weirdly, it's a very specific change, and further - is it actually a bottleneck anywhere to copy the vector this way?

vladislavbelov added inline comments.
source/gui/CChart.cpp
200

This change add a strong restriction, you should call UpdateSeries only after the data was changed by a script. I suppose it may be broken. @elexis could you test a case when you updated data once and then update axis_width/size/style multiple times.

source/scriptinterface/ScriptConversions.cpp
349

That's useless and looks strange.

source/scriptinterface/ScriptConversions.h
39

u32 > size_t.

elexis planned changes to this revision.Aug 11 2019, 4:36 PM
elexis added inline comments.
source/gui/CChart.cpp
200

The specs say (

17.6.5.15 Moved-from state of library types [lib.types.movedfrom]
Objects of types defined in the C++ standard library may be moved from (12.8). Move operations may be explicitly specified or implicitly generated. Unless otherwise specified, such moved-from objects shall be placed in a valid but unspecified state.

)
so unless it bugs in C++ due to chained calls, this unspecified behavior could be passed on to JS in theory.
Perhaps one could also swap vectors.
Or have m_Points use a const ref....

But actually, why does m_Series exist to begin with - it sounds like it should be stored in the SettingPointer already.

One can use series = { values: [], colors: [] } and then parse the color in FromJSVal without copying, even without creating the temporary, without having the member variable at all, unless I miss something.

source/scriptinterface/ScriptConversions.h
39

(JS_SetElement expects unsigned int, but one may prefer size_t since size() is in it and since the val[i] uses that, rather than the destination JS_SetElement, whatever)

44

The index.