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

 
 

patch #10147: interpreter: Avoid string construction

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

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 28 Feb 2023 02:48:49 PM UTC, comment #8: 

I see now that the second patch is also patch #10217 and the third patch is also patch #10218.  Since the first is also irrelevant now, I'm closing this patch but referencing it in the comments of patches 10217 and 10218.

John W. Eaton <jwe>
Group administrator
Tue 28 Feb 2023 02:20:54 PM UTC, comment #7: 

The first patch is irrelevant now because the functions like _get_load_path_ no longer accept arguments.

For the second patch, I generally dislike returning pointers or references to internal data members, even if they are constant because it means the programmer needs to ensure that the lifetime of the containing object is longer than the reference to the internal data, and that there are no changes to the containing object that will invalidate the reference or change the value that it refers to unexpectedly.  It seems to me that it is easy to miss details like this when refactoring code in the future, and that can lead to hard to find bugs.  To me, the possible maintenance issues outweigh the performance gains.  Maybe there is another way to do this job that doesn't have the possible maintenance issues?

In the third patch, I agree that setting retval = "" (the first part of the diff) is unnecessary, but setting dir_name = "" in the other two bits of the diff ensures that dir_name is empty if no valid function is found.  That seems reasonable to me, as the user of the function might have passed something else and setting it to "" could avoid later confusion.  I understand these are "internal" functions and that maybe fcn_info "is not supposed to be used that way" but I tend to worry about future maintenance.

John W. Eaton <jwe>
Group administrator
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>
Group 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 <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>
Group 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 <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 <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>
Group administrator
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 <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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-02-28 jwe StatusNeed Info Duplicate
        Open/ClosedOpen Closed
    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.12.
    Corresponding source code