Implement CString as a wrapper around a C-String to avoid using std::string #107

Flamefire · 2020-07-08T13:33:06Z

This avoids unnecessary memory allocations by wrapping the raw pointer (and the length).

I did some experiments using the 2 Benchmark PRs and the simpleFunc BM.

Master:

 100000: Range=1.1948-1.6665 Mean=1.3541 Median=1.3415
1000000: Range=6.1907-6.8819 Mean=6.4104 Median=6.3628

CString w/o length:

  100000: Range=1.1841-1.5298 Mean=1.3112 Median=1.3075
 1000000: Range=5.9802-6.4112 Mean=6.1600 Median=6.1523

CString w/ length:

 100000: Range=1.0298-1.6591 Mean=1.2913 Median=1.2709
1000000: Range=5.8449-6.4538 Mean=6.1164 Median=6.1250

CString w/ length and mem* functions:

 100000: Range=1.1546-1.5333 Mean=1.3124 Median=1.3039
1000000: Range=5.7451-6.4505 Mean=6.0884 Median=6.0576

So yes it does get faster although the effect in this limited scenario is limited.

AndreasGocht · 2020-07-15T14:20:57Z

Your call. If you like to have it in the master please fix the conflicts that occurred after merging #105 .

Flamefire · 2020-07-20T16:12:11Z

Rebased to master to fix the conflicts. Had to squash the commits into 1 as there were to many conflicts and new functions so most commits would have been pointless.

AndreasGocht

There are a few design decisions I do not understand. For example:

Why do you have a CString and a PythonCString class? Arent Python C-Strings the only thing we are dealing with?
Why does make_region_name still return a std::string?

I have to admit, I am not happy having now four different string representations:

std::string
classical c strings
CString
PythonCString

I think we should either move to only PythonCStrings for anything string related or we stay with the current approach.
Best,

Andreas

AndreasGocht · 2020-07-22T08:34:35Z

src/scorepy/events.cpp

-        SCOREP_User_RegionSetGroup(region_handle.value,
-                                   std::string(module, 0, module.find('.')).c_str());
+        const auto& region_name = make_region_name(module, function_name);
+        region_handle = create_user_region(region_name, module, file_name, line_number);


Please either rename to scorep_create_user_region or use the initial code here, what I would prefer, as we would have all the Score-P calls on one level.

The point is that we duplicate the code on how a user region is created and that code is slightly more than the 2 SCOREP_* calls if you want to avoid useless memory allocations: std::string(module, 0, module.find('.')) copies the string even when module was a std::string already and no dot is found which is what this PR strives to avoid. Encapsulating duplicate code is what functions are for.

And why scorep_create_user_region? What is not clear from the current name? And why the scorep_ prefix which makes it look like a Score-P call while it is a scorepy call creating a user region. Maybe create_scorep_user_region if you want to be more explicit?

AndreasGocht · 2020-07-22T08:36:44Z

src/scorepy/pythonHelpers.hpp

+
+/// Pair of a C-String and it's length useful for PyArg_ParseTuple with 's#'
+/// Implicitely converts to CString.
+struct PythonCString


Why not merging CString and PythonCString to PythongCString?

PythonCString is a workaround for the situation where you need a const char* and size pair. CString is a NULL-terminated sequence of chars with a length. According to rules of encapsulation you must not have write access to the members of CString as that could destroy invariants, in this case that len is the length of s. This doesn't hold for PythonCString which is simply for getting a CString from Python using the PyArg_ParseTuple function and unsafe to use further. Hence to be used only directly at the API boundary.

AndreasGocht · 2020-07-22T08:50:27Z

src/scorepy/cInstrumenter.cpp

-        assert(module_name);
-        // TODO: Use string_view/CString comparison?
-        if (std::string(name) != "_unsetprofile" && std::string(module_name, 0, 6) != "scorep")
+        const CString name = get_string_from_python(*code.co_name);


What about an Initalisert, which takes the frame?

Can you elaborate?

AndreasGocht · 2020-07-22T08:51:51Z

src/scorepy/events.cpp

 #endif
 };

+static region_handle create_user_region(const std::string& region_name, const CString module,


why std::string for region_name?

Because this is what you have when calling the function. I could add an overloaded constructor to CString so it can be implicitely (or better explicitely) converted from a std::string if you like.

AndreasGocht · 2020-07-22T08:54:36Z

src/scorepy/events.cpp

+
+    // Extract main module name if module is like "mainmodule.submodule.subsubmodule"
+    const char* dot_pos = module.find('.');
+    if (dot_pos)


I would prefer to move this to PythonCString.

Can you explain what you want to move exactly? Keep in mind that allocations should be avoided hence a string copy must only be done when required, in this case when a dot was found

AndreasGocht · 2020-07-22T08:57:10Z

src/scorepy/events.hpp

 /// Combine the arguments into a region name
 /// Return value is a statically allocated string to avoid memory (re)allocations
-inline const std::string& make_region_name(const std::string& module_name, const std::string& name)
+inline const std::string& make_region_name(const CString module_name, const CString name)


As written I would vote for implementing this returning PythonCString

The result is used to search a map with keys of std::string. So if this should return a CString then CString would need to be implicitely convertible from const std::string& to work with that. Shall I do that?

AndreasGocht · 2020-07-22T08:58:24Z

src/scorepy/pythonHelpers.hpp

    return reinterpret_cast<detail::ReplaceArgsToPyObject_t<TFunc>*>(func);
 }

+inline CString get_string_from_python(PyObject& o)


Add this as function, or initialiser of PythonCString

I can do that. However the intention of CString was to keep it generic. This separates concerns. Adding this function would tie it to Python

What is wrong with this function?

Flamefire · 2020-07-23T14:26:14Z

Why do you have a CString and a PythonCString class? Arent Python C-Strings the only thing we are dealing with?

CString is a generic wrapper around a C String to a) make working with it easier (see comparison and starts_with) and b) avoid passing around const char* which you disliked.
PythonCString is an interface class which bridges a gap between Python and CString, see #107 (comment) Not a big fan, but at least it simplifies things like foo_str, foo_len pairs and their conversion to CString which is error-prone.

Why does make_region_name still return a std::string?

Because that is what is need for the map where it is used. See #107 (comment) for an alternative

I have to admit, I am not happy having now four different string representations:

Correct me if I'm wrong but there are no "classical c strings" used anymore (except as sinks in SCOREP_* functions) and PythonCString is the opposite: Another interface type only used for ParseTuple. So you have only CString as a non-owning string and std::string as an owning string which sounds perfectly valid.

Flamefire force-pushed the cstring branch from 484caf1 to 639b137 Compare July 20, 2020 16:11

AndreasGocht requested changes Jul 23, 2020

View reviewed changes

Flamefire added 2 commits April 21, 2021 11:06

Add CString class to avoid allocations

132d21c

Make get_string_from_python Python2 compatible

8556089

Flamefire force-pushed the cstring branch from 7eb13a2 to 8556089 Compare April 21, 2021 09:10

Implement CString as a wrapper around a C-String to avoid using std::string #107

Are you sure you want to change the base?

Implement CString as a wrapper around a C-String to avoid using std::string #107

Uh oh!

Conversation

Flamefire commented Jul 8, 2020

Uh oh!

AndreasGocht commented Jul 15, 2020

Uh oh!

Flamefire commented Jul 20, 2020

Uh oh!

AndreasGocht left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flamefire commented Jul 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants