patchGNU Octave - Patches: patch #10139, interpreter: static instead of...

 
 

patch #10139: interpreter: static instead of dynamic casts for speed

Submitted by:  Petter Tomner <petter>
Submitted on:  Sun 21 Nov 2021 04:56:21 PM UTC
 
Category:  Core : other Priority:  5 - Normal
Status:  In Progress Privacy:  Public
Assigned to:  None Open/Closed:  Open
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

Wed 30 Mar 2022 09:29:27 PM UTC, comment #2: 

Ye maybe it could flip from default dynamic to static in a major release or something. dynamic_cast is surprisingly slow.

I don't think lib users depend on the bad_cast exception, since typeid need to be messed up somehow for the octave_value object to make the lookup get the wrong functions (I think).

I look into your feedback at some point.

I've done a lot of similar speedup patches as a byproduct of a try to make a stack based interpreter for Octave. Some might be interesting to look at but I believe a lot breaks lib ABI.

https://savannah.gnu.org/patch/?10167

Petter Tomner <petter>
Sun 13 Mar 2022 06:20:22 PM UTC, comment #1: 

Sorry for taking so long for coming around to this.

This is definitely interesting. I'm not 100% sure if a static_cast will always be safe. But that seems to be the case afaict (at least for Octave's own codebase).
And I like the approach of optionally turning the "dynamic_cast" back on.

Just a minor comment: Should the new macro be renamed from DYNORSTAT_CAST to something like OCTAVE_OBV_CAST to avoid potential clashes of the macro name when the `ops.h` header is included in downstream projects?
IIUC, these casts are all between types that are derived from `octave_base_value`. Hence, the "OBV" in the proposed macro name.
But other names are also ok as long as they start with "OCTAVE_" - I guess.

Also: Should the configure switch apply to downstream projects that include `ops.h`? In that case, OCTAVE_ENABLE_INTERNAL_CHECKS should probably also be included in `oct-conf-post-public.in.h`.
Alternatively, we could default to `dynamic_cast` for user code if we are uncertain what could happen downstream. Perhaps with the option to explicitly switch to `static_cast` by passing `-DOCTAVE_NO_INTERNAL_CHECKS` or something similar...

I'm attaching an updated patch that applies on the current default branch.

(file #52986)

Markus M├╝tzel <mmuetzel>
Project Member
Sun 21 Nov 2021 04:56:21 PM UTC, original submission:  

Hi!

I've made a patch trying to speed up the interpreter by replacing dynamic_cast with static_cast.

I added a configure flag "--enable-internal-checks" which could be used to enable expensive runtime checks.

In ops.h I added a macro DYNORSTAT_CAST that is static_cast by default and dynamic_cast if Octave is built with "internal-checks".

In the operator cc-files a changed all the dynamic_cast of references to DYNORSTAT_CAST.

As I understand the code, the objects used by the operators are not supposed to be of the wrong kind. A std::bad_cast is thrown currently which terminates Octave, if that is the case, but with this patch any type error would silently corrupt memory etc.

For scalar heavy code with many ops the speedup is quite substantial. E.g. some silly:

function i = foo ()
        tic;
        for i = 1:10e6
                i = 0.5 * i / 0.9 + 1 - 8;
        end
        toc;
end

Has a speedup of about 15% on my machine.

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 #52315:  staticcast_tomner.patch added by petter (231KiB - text/x-patch)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by siko1056
  • -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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-03-13 mmuetzel Attached File- => Added patch10139-staticcast_tomner-v2.patch, #52986
        StatusNone => In Progress
    2022-03-07 mmuetzel Dependencies- => bugs #62146 is dependent
    2021-12-21 siko1056 Carbon-Copy- => Added siko1056
    2021-11-21 petter Attached File- => Added staticcast_tomner.patch, #52315

    Back to the top


    Powered by Savane 3.9