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 25 Jun 2013 01:01:53 AM UTC  
 
Category: NonePriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: NoneOpen/Closed: Closed

Add a New Comment(Rich 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 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>
Project 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>
Project 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>
Project Administrator
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>
Project 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>
Project Administrator
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

by

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>
Project 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>
Project Administrator
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 File(s):
   
   
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
  • -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 09 Jun 2014 02:54:50 AM UTCrik5StatusIn Progress=>Done
      Open/ClosedOpen=>Closed
    Sun 08 Jun 2014 11:35:07 PM UTCjalanpalmerAttached File-=>Added isscalar_built-in.diff, #31522
    Wed 26 Jun 2013 12:53:01 AM UTCjalanpalmerAttached File-=>Added isscalar_built-in.diff, #28421
    Tue 25 Jun 2013 08:01:44 PM UTCjordighStatusNone=>In Progress
    Tue 25 Jun 2013 01:01:53 AM UTCjalanpalmerAttached File-=>Added isscalar_built-in.diff, #28405

    Back to the top


    Powered by Savane 3.1-cleanup1