patchGNU Octave - Patches: patch #8093, Convert isscalar to a built-in...


patch #8093: Convert isscalar to a built-in function

Submitted by:  Jason Alan Palmer <jalanpalmer>
Submitted on:  Tue Jun 25 01:01:53 2013  
Category: NonePriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: NoneOpen/Closed: Closed

Add a New Comment (Rich MarkupRich Markup):

You are not logged in

Please log in, so followups can be emailed to you.


(Jump to the original submission Jump to the original submission)

Mon Jun 9 02:54:50 2014, comment #13:

I applied your changeset to the development branch here ( Only thing I did was adjust the indent from 4 spaces to 2 spaces which is the Octave convention. I also added your name to the list of contributors to Octave which is displayed at the front of the manual.

Thanks for the patch.

Rik <rik5>
Project Administrator
Sun Jun 8 23:35:07 2014, comment #12:

Since the previous patch no longer applied cleanly, I have fixed it and re-exported.

(file #31522)

Jason Alan Palmer <jalanpalmer>
Wed May 28 16:34:04 2014, comment #11:

I've filed an issue report to re-code the most frequently used predicate tests (such as isscalar) into C++ for release 4.2. See bug #42422.

Rik <rik5>
Project Administrator
Sat Aug 3 15:12:30 2013, comment #10:

In previous comments I already conceded that the isscalar calls are in my own code and that another octave function is not to blame. I use it for arg-checking in a lot of my functions, along with some other is... functions.

Jason Alan Palmer <jalanpalmer>
Wed Jul 31 20:19:34 2013, comment #9:

Well, if isscalar is slow, that's an actual problem that needs to be fixed. I would prefer a solution that doesn't involve turning it into a C++ function, that's all.

But reading the m-file, I don't see how it could be made any faster in m-code. If you're calling this function millions of times, then I can see how turning it into C++ could make it go faster. So under what situation are you calling isscalar millions of times? Is another Octave function to blame? Can we fix that in m-code instead?

Jordi Gutiérrez Hermoso <jordigh>
Project Administrator
Wed Jul 31 20:00:39 2013, comment #8:

I'm not sure that this particular change is a bad one. Moving every .m file function to C++ doesn't make sense but I'm not fundamentally opposed to moving trivial functions like this to C++.

John W. Eaton <jwe>
Project Administrator
Wed Jul 31 19:48:42 2013, comment #7:

I try not to leave open entries in trackers, but it looks like I can't close my own entries here?

My only request before you close is for someone to consider updating to include a mention of the preference for m-files over C++.

Jason Alan Palmer <jalanpalmer>
Thu Jun 27 13:32:26 2013, comment #6:

C++ scares new contributors (hell, it scares me too sometimes). It's a much more clunky language than the Octave language for getting things done. So we prefer if as much of our code as possible is written in m-files.

When JIT compiling gets better in Octave, I will start to investigate moving functions out of C++ and into m-files.

Jordi Gutiérrez Hermoso <jordigh>
Project Administrator
Thu Jun 27 13:21:34 2013, comment #5:

The call to isscalar was in my own code. Yes, I see that using numel directly would be faster, since it's built-in.

I'm still curious about the "we try to avoid writing functions in C++ whenever possible" policy, though. What are the tradeoffs of keeping simple functions as m-files versus building them in?

Jason Alan Palmer <jalanpalmer>
Thu Jun 27 13:06:53 2013, comment #4:

Is the call to "isscalar" in your own code or in a core/package function?
If it is your own code try to substitute


to avoid interpreter overhead.

If it is in a function from Octave-core or from a package, could
you specify what that function is?

Carlo de Falco <cdf>
Project Member
Wed Jun 26 00:53:01 2013, comment #3:

I wasn't aware that C++ functions are frowned upon. I don't have any particular case where I'd call isscalar a bottleneck, just some scripts where it is heavily used and gets pretty high on my profile results:

Function Time (s) Calls
some_function 133.64 1075200
another_function 133.219 1075200
isscalar 119.322 12906470
more_function 114.951 1075200
my_function 107.563 800
(I realize it got up there because it was called over ten million times....)

A quick check shows the built-in to be about 4x as fast on my machine.

(file #28421)

Jason Alan Palmer <jalanpalmer>
Tue Jun 25 20:01:44 2013, comment #2:

We try to avoid writing functions in C++ whenever possible, and I'm surprised that you found the m-file implementation of isscalar too slow. Do you have an example of where this turned into a bottleneck?

Jordi Gutiérrez Hermoso <jordigh>
Project Administrator
Tue Jun 25 19:32:21 2013, comment #1:

Whoops, I posted the wrong patch, not following your committing and coding standards! Please ignore this patch until I resubmit it later today....

Jason Alan Palmer <jalanpalmer>
Tue Jun 25 01:01:53 2013, original submission:

This simple patch converts the isscalar function from an m-file to a built-in.

I ran 'make check' before and after the change: all tests still pass.

(FYI: I made this because I was profiling some of my own code and saw that a lot of time was spent in isscalar and friends due to the arg-checking I was doing. If this patch is considered correct, then I'll have a list of other functions to give the same treatment to.)

Jason Alan Palmer <jalanpalmer>


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

Attach File(s):

Attached Files
file #31522:  isscalar_built-in.diff added by jalanpalmer (3kB - text/plain - diff from hg export)
file #28421:  isscalar_built-in.diff added by jalanpalmer (3kB - application/x-download - diff from hg export (corrected))
file #28405:  isscalar_built-in.diff added by jalanpalmer (3kB - application/x-download - diff from hg export)


Depends on the following items: None found

   bug dependencies.


Carbon-Copy List
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by cdf (Posted a comment)
  • -unavailable- added by jordigh (Posted a comment)
  • -unavailable- added by jalanpalmer (Submitted the item)

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.


    Please enter the title of George Orwell's famous dystopian book (it's a date):



    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon Jun 9 02:54:50 2014rik5StatusIn Progress=>Done
    Sun Jun 8 23:35:07 2014jalanpalmerAttached File-=>Added isscalar_built-in.diff, #31522
    Wed Jun 26 00:53:01 2013jalanpalmerAttached File-=>Added isscalar_built-in.diff, #28421
    Tue Jun 25 20:01:44 2013jordighStatusNone=>In Progress
    Tue Jun 25 01:01:53 2013jalanpalmerAttached File-=>Added isscalar_built-in.diff, #28405

    Back to the top

    Powered by Savane 3.1-cleanup1