bugGNU Octave - Bugs: bug #61030, [octave forge] (image) Don't use...

 
 

bug #61030: [octave forge] (image) Don't use internal function

Submitted by:  Markus Mützel <mmuetzel>
Submitted on:  Wed 11 Aug 2021 05:03:06 PM UTC  
 
Category:  Octave Forge Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Build Failure
Status:  In Progress Assigned to:  Avinoam Kalma <avinoam>
Originator Name:  Open/Closed:  Open
Release:  dev Operating System:  Any

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)

Mon 23 Aug 2021 07:59:16 AM UTC, comment #13: 

The configure test was always failing afaict. So, it never used (the more efficient) `xelem`.

The attached updated patch should be fixing that.

(file #51807)

Markus Mützel <mmuetzel>
Project Member
Sun 22 Aug 2021 01:58:52 PM UTC, comment #12: 

Thanks, @Markus,
this patch compiles under 4.2.0

I will make some tests to check the patch.

Avinoam Kalma <avinoam>
Project MemberIn charge of this item.
Sun 22 Aug 2021 12:33:14 PM UTC, comment #11: 

Thanks, @Markus, I will check the new patch

Avinoam Kalma <avinoam>
Project MemberIn charge of this item.
Sun 22 Aug 2021 12:28:17 PM UTC, comment #10: 

The attached patch adds a configure check to test if the `dim_vector` class has an `xelem` member function. If it doesn't, it would fall back to the `elem` function (with the additional cost of boundary checking).

I haven't tested at all, so it might need some tinkering. If it works, it should define HAVE_OCTAVE_DIM_VECTOR_XELEM if that function exists.

I also don't know the coding style of the image package. I hope it is ok.

(file #51803)

Markus Mützel <mmuetzel>
Project Member
Sun 22 Aug 2021 11:47:32 AM UTC, comment #9: 

On ver 4.4.1 the compilation is OK

Avinoam Kalma <avinoam>
Project MemberIn charge of this item.
Sun 22 Aug 2021 11:35:35 AM UTC, comment #8: 

Thanks, @Hartmut.

testing on 4.2.0 I get

In file included from connectivity.cc:17:0:
connectivity.h: In static member function 'static void octave::image::connectivity::set_padding(cons
t dim_vector&, const dim_vector&, T&, const P&)':
connectivity.h:156:43: error: 'const class dim_vector' has no member named 'xelem'
   const octave_idx_type row_stride = size.xelem (0);
                                           ^

So, this fix will not work 4.2.0.
Should continue to support this version?

Avinoam Kalma <avinoam>
Project MemberIn charge of this item.
Wed 18 Aug 2021 07:31:20 PM UTC, comment #7: 

For the (only nice to have) test of the excution speed, you could use something quite simple like this:

clear

tic
for nloop = 1:100
test bwconncomp
endfor
time_bwconncomp = toc/nloop

tic
for nloop = 1:3
test imreconstruct
endfor
time_imreconstruct = toc/nloop

Running this (probably only) in the current core Octave release version, one time with the current image package release, and a second time (on the very same pc) with the "remove jit" patch applied to the image package (and properly re-compiled) should be enough to be sure that there are no "dramatic" speed regressions. We could then conclude something like "the speed decrease in the relevant BISTs is no more than x% with this patch applied." I know that BISTs are not made for this, but it would be quite easy to do it like this.

Hartmut <hardy>
Wed 18 Aug 2021 04:22:36 AM UTC, comment #6: 

Thanks, @Hartmut for the detailed review.

Some clarification questions and answers:

  • Commit message: the bug report number and the file names of all touched source code files are already mentioned.
  • Markus Mützel is the author of this patch.
  • I will check compilation and BISTs in older versions.
  • Could you please recommend how to check the run-time?

Thank again

Avinoam Kalma <avinoam>
Project MemberIn charge of this item.
Mon 16 Aug 2021 03:06:41 PM UTC, comment #5: 

I am not very fluent in C, but I would like to see the following formal changes, or have the following formal checks done and reported here, before pushing this patch:

  • improve the wording of the commit message of the patch, e.g. mention the bug report number, and list the file names of all touched source code files. All as usual.
  • name Markus Mützel as author of this patch in the commit (as long as we mostly just copy his code changes over to the image repo).
  • Please check that the image package still compiles in all core Octave versions that we currently still support. (I assume that it compiles fine in current dev Octave, since the Octave CI setups don't complain. But does it also compile fine in Octave version 4.2.0, the oldest Octave core version still supported by the image package?)
  • Please check that the corresponding BISTs all still pass (at least the BISTs for the three files touched, and at least for Octave 4.2.0. Probably also in Octave 6.3.0 and 5.2.0 and 4.4.1)
  • Ideally (but optionally) do a small check if there is a (significant) run-time increase with this code change. If yes, we probably cannot do anything about it, it would just be nice to know.

I have recently done a full OS re-installation on my laptop, and won't be able to do those tests on my own machine in a reasonable time frame.

Soon I will also be offline for a couple of weeks. But as soon as the above items are check, I would be happy if someone (Avinoam?) pushes this patch to the repo.

Hartmut <hardy>
Sat 14 Aug 2021 07:25:14 PM UTC, comment #4: 
Avinoam Kalma <avinoam>
Project MemberIn charge of this item.
Fri 13 Aug 2021 05:07:16 PM UTC, comment #3: 

The to_jit appears in
connectivity.h
bwconncomp.cc
imreconstruct.cc

I see that the patch in comment #0 handle this, so I will make a
proper changeset.

Thanks.

Avinoam Kalma <avinoam>
Project MemberIn charge of this item.
Thu 12 Aug 2021 06:23:28 AM UTC, comment #2: 

Thanks for letting us know, Markus. I'm adding people to cc for now.

Hartmut <hardy>
Wed 11 Aug 2021 06:11:21 PM UTC, comment #1: 

Forgot to add a link to the changeset that removed this function in Octave:
https://hg.savannah.gnu.org/hgweb/octave/rev/f254c302bb9c

Markus Mützel <mmuetzel>
Project Member
Wed 11 Aug 2021 05:03:06 PM UTC, original submission:  

The image package is using the function `dim_vector::to_jit` which was marked for internal usage with a comment in the header file. That function will finally be removed in Octave 7.

Consider replacing that function by accessing elements with `dim_vector::xelem` instead. That function should be equally efficient already with a very low level of compiler optimization.

A possible patch is in MXE Octave here:
https://hg.octave.org/mxe-octave/file/93d20ff5d6a6/src/of-image-1-no-jit.patch

Markus Mützel <mmuetzel>
Project Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51807:  bug61030-v3.patch added by mmuetzel (6KiB - application/octet-stream)
file #51803:  bug61030-v2.patch added by mmuetzel (6KiB - application/octet-stream)
file #51778:  bug61030.cs added by avinoam (5KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by hardy (Posted a comment)
  • -email is unavailable- added by hardy
  • -email is unavailable- added by hardy
  • -email is unavailable- added by mmuetzel (Submitted the item)
  •  

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

    Only project members can vote.

     

     

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-08-23 mmuetzel Attached File- => Added bug61030-v3.patch, #51807
    2021-08-22 mmuetzel Attached File- => Added bug61030-v2.patch, #51803
    2021-08-18 avinoam StatusConfirmed => In Progress
    2021-08-14 avinoam Attached File- => Added bug61030.cs, #51778
    2021-08-13 avinoam StatusNone => Confirmed
        Assigned toNone => avinoam
    2021-08-12 hardy Carbon-Copy- => Added avinoam
        Carbon-Copy- => Added carandraug

    Back to the top


    Powered by Savane 3.6