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.
|
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)
|
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.
|
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.
|
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?
|
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++.
|
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++.
|
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.
|
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?
|
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.
|
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)
|
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?
|
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....
|
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.)
|