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

 
 

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

Submitter:  Jason Alan Palmer <jalanpalmer>
Submitted:  Tue 25 Jun 2013 01:01:53 AM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 09 Jun 2014 02:54:50 AM UTC, comment #13: 

I applied your changeset to the development branch here (http://hg.savannah.gnu.org/hgweb/octave/rev/bc52657a7d29).  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>
Group administrator
Sun 08 Jun 2014 11:35:07 PM UTC, comment #12: 

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

(file #31522)

Jason Alan Palmer <jalanpalmer>
Wed 28 May 2014 04:34:04 PM UTC, 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>
Group administrator
Sat 03 Aug 2013 03:12:30 PM UTC, 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 31 Jul 2013 08:19:34 PM UTC, 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>
Group Member
Wed 31 Jul 2013 08:00:39 PM UTC, 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>
Group administrator
Wed 31 Jul 2013 07:48:42 PM UTC, 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 http://www.gnu.org/software/octave/doc/interpreter/Contributing-Guidelines.html to include a mention of the preference for m-files over C++.

Jason Alan Palmer <jalanpalmer>
Thu 27 Jun 2013 01:32:26 PM UTC, 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>
Group Member
Thu 27 Jun 2013 01:21:34 PM UTC, 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 27 Jun 2013 01:06:53 PM UTC, 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


if (isscalar (x));


by


if (numel (x) == 1);


to avoid interpreter overhead.

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

Carlo de Falco <cdf>
Group Member
Wed 26 Jun 2013 12:53:01 AM UTC, 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 25 Jun 2013 08:01:44 PM UTC, 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>
Group Member
Tue 25 Jun 2013 07:32:21 PM UTC, 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 25 Jun 2013 01:01:53 AM UTC, 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 Files:
   
   
Comment:
   

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

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by cdf (Posted a comment)
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by jalanpalmer (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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-06-09 rik5 StatusIn Progress Done
        Open/ClosedOpen Closed
    2014-06-08 jalanpalmer Attached File- Added isscalar_built-in.diff, #31522
    2014-05-28 rik5 Dependencies- bugs #42454 is dependent
    2013-06-26 jalanpalmer Attached File- Added isscalar_built-in.diff, #28421
    2013-06-25 jordigh StatusNone In Progress
    2013-06-25 jalanpalmer Attached File- Added isscalar_built-in.diff, #28405

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code