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: In ProgressPrivacy: Public
Assigned to: NoneOpen/Closed: Open

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)

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 #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

Items that depend on this one: None found

 

Carbon-Copy List
  • -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 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    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-cleanup