patchGNU Octave - Patches: patch #10147, interpreter: Avoid string...

 
 

patch #10147: interpreter: Avoid string construction

Submitter:  Petter Tomner <petter>
Submitted:  Fri 26 Nov 2021 02:01:29 PM UTC
 
Category:  Core : other Priority:  5 - Normal
Status:  Need Info Privacy:  Public
Assigned to:  None Open/Closed:  Open
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( Jump to the original submission )

Thu 28 Apr 2022 11:23:44 PM UTC, comment #6: 

What is the status of these patches? Are they ready to apply and test?

Arun Giridhar <arungiridhar>
Project Member
Tue 28 Dec 2021 01:03:06 AM UTC, comment #5: 

[comment #4:]

> The first change seems OK, at least in functions that are called many many times, but I'm not interested in changing all Octave code to use static const std::string objects for every string literal.  The context matters.  For example, there is no need for it in something like
>

> if (some_error_condition)
>   error ("...");


Ye that would be silly. The _get* function signatures could instead be changed to 'const * char' to avoid the const static std::string, since they are just used for a
'.c_str ()' in the end anyway.

> I don't think the other two changes are needed.  I'd rather avoid adding things like "m_empty_string" member variables or const reference for ordinary local variables.  Shouldn't std::move be used in those instances anyway, or does something prevent that from happening?
>


I think std::move is used (the move constructor) but I think there need to be a malloc for the returned string anyways somewhere to have a copy. The shenanigans
with the "m_empty_string" is to allow a const reference for the "return m_rep ? m_rep->m_foo : m_empty_str". A copy of a empty string should not result
in any mallocs due to "short string optimizations", but a copy of m_foo might trigger one depending on size.

I tried patch 2 in isolation and verified that it removes two mallocs per for loop iteration. One because of changes in symscope and one because of changes in fcn-info.

Removing const ref locals in fcn-info.cc with fcn-info.hh as in patch 2 did not remove the malloc.

Petter Tomner <petter>
Wed 22 Dec 2021 02:38:35 PM UTC, comment #4: 

The first change seems OK, at least in functions that are called many many times, but I'm not interested in changing all Octave code to use static const std::string objects for every string literal.  The context matters.  For example, there is no need for it in something like

if (some_error_condition)
  error ("...");

I don't think the other two changes are needed.  I'd rather avoid adding things like "m_empty_string" member variables or const reference for ordinary local variables.  Shouldn't std::move be used in those instances anyway, or does something prevent that from happening?

John W. Eaton <jwe>
Project Administrator
Sat 18 Dec 2021 02:16:18 PM UTC, comment #3: 

I have split up the patch accordingly:

Patch 1 - static strings for some _get* functions in fcn-info.cc

Patch 2 - Some const string references instead of copies

Patch 3 - Remove some redundant ""-writes

I ditched the query/process code since it got really kludgy trying to bubble up the result
and it didn't do too much good.

Also I ditched the unordered_map:s since the speed improvement seemed dependent on other patches.

Baseline:

function i = foo ()
  tic;
  for i = 1:100000
    i = 1+2+3+4+5+6+sin(7);
  end
  toc;
end

  37 mallocs per loop iteration

Patch 1:
  Runs about 5% faster than baseline.
  -9 mallocs per loop iteration

Patch 2:
  Unmeasurable speed improvement. 3% according to callgrind.
  -2 mallocs per iteration

Patch 3:
  Unmeasurable speed improvement but verified removes some writes. 0.6% according to callgrind

In total:
  About 5% faster measured. Depends alot on exactly what code is executed. 13% fewer cpu cycles
  according to callgrind.

_run_test_suite_ runs fine* on Debian x64.

  PASS                            16970
  FAIL                                0
  XFAIL (reported bug)               31
  SKIP (missing feature)            155
  SKIP (run-time condition)          26

*(I had to disable eigs' tests since those segfaults on my machine, with or without these patches).

(file #52529, file #52530, file #52531)

Petter Tomner <petter>
Tue 14 Dec 2021 09:15:58 PM UTC, comment #2: 

OK I'll split the patch up and post them here. I'll rebase to HEAD too.

I missed the tab rule. I'll untabify.

>  Does it compile for you? It looks like the fcn_info_rep constructor is missing an argument. Or I can't see where `m_file_name` gets actually set...

Yes it compiled, even though I am new to hg so I might have messed something up.

'm_full_name' is set in the constructor initializer list. It is impossible to see from the patch diff that 'name' is split up into 'package_name' and back into 'name' in the ctor. So 'name' is initially the full name. (I should have used 'nm' instead for setting m_full_name for clarity.)

      fcn_info_rep (const std::string& nm)
        : name (nm), package_name (), m_full_name (name), local_functions (),
          /* ... */
      {
        std::size_t pos = name.rfind ('.');
        if (pos != std::string::npos)
          {
            package_name = name.substr (0, pos);
            name = name.substr (pos+1);
          }
      }

Petter Tomner <petter>
Tue 14 Dec 2021 03:13:05 PM UTC, comment #1: 

This patch is quite large which makes it difficult to review.

Could you please split it up in smaller changes. E.g., one that only changes some `map` objects to `unordered_map`s, another one that replaces string literals with `static const string`s, yet another one that makes `m_fullname` a new class member, ...
That would probably make it much easier to digest.

Additionally, we try to avoid tabulators for white space in code. They display differently for different editors or settings. I've seen pretty much everything from 2 characters to 16 characters per tab... (Is it 8 for your editor?)

I didn't try that patch yet.
Does it compile for you? It looks like the fcn_info_rep constructor is missing an argument. Or I can't see where `m_file_name` gets actually set...

Markus M├╝tzel <mmuetzel>
Project Member
Fri 26 Nov 2021 02:01:29 PM UTC, original submission:  

Hi!

There are alot of string construction and destruction in the interpreter that are unnecessary.

I split up the function look-up functions in a querry and process part to avoid constructing strings if no function were found (which are alot since multiple tables are queried).

Also, I replaced some string literals with static const std::string to avoid the container overhead constructing a std::string each iteration.

Code like:

for i = 1:100000
        i = sin (i);
end

requires about 34 malloc calls per loop iteration. With this patch I got it down to 23. Which makes it about 16% faster.

The function fcn-info.cc::get_dispatch_type still makes strings. Changing it in this patch would make it abit too big.

Petter Tomner <petter>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #52529:  avoidstringctors_1.patch added by petter (4KiB - text/x-patch)
file #52530:  avoidstringctors_2.patch added by petter (5KiB - text/x-patch)
file #52531:  avoidstringctors_3.patch added by petter (1018B - text/x-patch)
file #52346:  avoidstringctors_pt.patch added by petter (28KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by siko1056 (Updated the item)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by petter (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

     

     

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-12-18 petter Attached File- Added avoidstringctors_1.patch, #52529
        Attached File- Added avoidstringctors_2.patch, #52530
        Attached File- Added avoidstringctors_3.patch, #52531
    2021-12-15 siko1056 CategoryNone Core : other
        StatusNone Need Info
    2021-11-26 petter Attached File- Added avoidstringctors_pt.patch, #52346

    Back to the top


    Powered by Savane 3.9