Performance improvements by aothms · Pull Request #47 · IfcOpenShell/IfcOpenShell

aothms · 2016-03-18T10:35:39Z

Re-use triangulated representation elements across products sharing representations by means of mapped items [1]
Options to manipulate caching behaviour [2]
Parent enumeration lookup in constant time [3]

[1]
Only in the cases listed below. Which seems to work for most windows and doors. But might cause instances not to be detected especially in MEP models where pipes are scaled uniformly and styled on a mapped-item level.

product.HasOpenings = { }

| rep.Items | = 1
rep.Items[0] ∈ IfcMappedItem
rep.Items[0].StyledByItem = { }
rep.Items[0].MappingTarget = [ [1 0 0 0] [0 1 0 0] [0 0 1 0] [0 0 0 1] ]
rep.Items[0].MappingSource.MappingOrigin = [ [1 0 0 0] [0 1 0 0] [0 0 1 0] [0 0 0 1] ]

[2]
From some early testing it seems that the caching of geometric elements is detrimental for performance. More analysis needed.

[3]
Thought compilers would optimize this, but they don't. Also removed potentially UB by relying on passing -1 as a NULL enumeration value. Using boost::optional now.

…rld coordinates

# Conflicts: # src/ifcconvert/ColladaSerializer.cpp # src/ifcconvert/ColladaSerializer.h # src/ifcgeom/IfcGeomIterator.h # src/ifcgeom/IfcGeomRenderStyles.h

aothms · 2016-03-22T15:21:35Z

+						if (ifcproduct_iterator == ifcproducts->begin() || settings.get(IteratorSettings::USE_WORLD_COORDS)) {
+							next_triangulation = new TriangulationElement<P>(*next_shape_model);
+						} else {
+							next_triangulation = new TriangulationElement<P>(*next_shape_model, current_triangulation->geometry_pointer());


@Stinkfist0 This does not work after merging, because with your take ownership flag current_triangulation == 0. Is this flag still necessary as the min,max bounds are now computed during initialization?

Let me test a get back to you soon.

Centering seems to work OK judging by a quick test:

Logger::Status("Creating geometry..."); if (center_model) { double* offset = serializer->settings().offset; gp_XYZ center = (context_iterator.bounds_min() + context_iterator.bounds_max()) * 0.5; offset[0] = -center.X(); offset[1] = -center.Y(); offset[2] = -center.Z(); //printf("Bounds min. (%g, %g, %g)\n", context_iterator.bounds_min().X(), context_iterator.bounds_min().Y(), context_iterator.bounds_min().Z()); //printf("Bounds max. (%g, %g, %g)\n", context_iterator.bounds_min().X(), context_iterator.bounds_min().X(), context_iterator.bounds_min().Z()); printf("Using model offset (%g, %g, %g)\n", offset[0], offset[1], offset[2]); //TODO Logger::Message(Logger::LOG_NOTICE, ...); } size_t num_created = 0; // The functions IfcGeom::Iterator::get() and IfcGeom::Iterator::next() // wrap an iterator of all geometrical products in the Ifc file. // IfcGeom::Iterator::get() returns an IfcGeom::TriangulationElement or // -BRepElement pointer, based on current settings. (see IfcGeomIterator.h // for definition) IfcGeom::Iterator::next() is used to poll whether more // geometrical entities are available. None of these functions throw // exceptions, neither for parsing errors or geometrical errors. Upon // calling next() the entity to be returned has already been processed, a // true return value guarantees that a successfully processed product is // available. do { const IfcGeom::Element<double>* geom_object = context_iterator.get(); ++num_created; if (is_tesselated) { serializer->write(static_cast<const IfcGeom::TriangulationElement<real_t>*>(geom_object)); } else { serializer->write(static_cast<const IfcGeom::BRepElement<real_t>*>(geom_object)); } const int progress = context_iterator.progress() / 2; if (old_progress != progress) Logger::ProgressBar(progress); old_progress = progress; } while (context_iterator.next()); Logger::Status("\rDone creating geometry (" + boost::lexical_cast<std::string>(num_created) + " objects) "); serializer->finalize(); delete serializer; Logger::Status("\rDone serializing geometry ");

…of objects

aothms · 2016-03-27T19:20:12Z

Ok, removed the option to take ownership again. Maybe something to look into is proper move and copy constructors? In case users still want this.

Stinkfist0 · 2016-03-28T09:01:12Z

Yeah, maybe look into this again if/when a use case rises.

Stinkfist0 · 2016-03-30T19:03:30Z

The results of this optimization are looking great (converted using --generate-uvs --sew-shells --use-element-guids --use-material-names):

Also in Unity, selecting two identical windows next to each other shows that both refer to the same mesh, as should be expected.

Stinkfist0 · 2016-04-08T14:38:05Z

I'm running now into issues with one file at least. The progress bar doesn't progress (context_iterator.progress() returns 0 always) even though while debugging I see geometry being created and num_created increasing steadily. I was able to convert this file OK with the master code. Unfortunately I can't share this file but the file consist of hundreds of mostly identical furnitures. I'll debug deeper and get back to you.

Stinkfist0 · 2016-04-08T14:58:30Z

Actually, I'm seeing the progress bar move now, I just had to wait couple minutes in order to see the first progress.

Stinkfist0 · 2016-04-08T16:41:29Z

Yeah, the conversion time went from 6 minutes (original code) to 30 (this PR) for this particular file (ENABLE_BUILD_OPTIMIZATIONS is off), but file size from OTOH 950 MB to 31 MB :)

aothms · 2016-04-09T11:22:35Z

Thanks for testing, but yikes, I'm not too happy with the increase in processing time. Would you mind checking whether this is due to disabling the cache? Could you post back the processing time with #define NO_CACHE on IfcGeom.h:49 commented out. Thanks in advance.

Stinkfist0 · 2016-04-24T09:54:16Z

Hi. Enabled the caching and the processing time went down to 37 seconds(!). What was the reasoning for disabling cache by default? I've been using this code for many models at work and everything seems to be in order - are you planning to merge this code soon?

aothms · 2016-04-24T10:13:44Z

Yeah this should be merged soon. Thanks for testing the caching. Caching accounts for a large part of the memory usage and in some of my tests the caching even proved to be detrimental in terms of processing time if there is little re-use, as it can take some time especially for large models to put everything in maps. Alternatively we can arbitrarily trash our cache once in a while (https://github.com/IfcOpenShell/IfcOpenShell/pull/47/files#diff-4a1897ad94043892537a522bdd7fafd5R328) to prevent memory footprint and cache insertion speed to grow out of control. I will do some more tests.

Note that this is a stop-gap measure anyway as in the future I hope to do this more precisely by determining re-use by means of counting inverse relationships and depleting cache entries as soon as possible. On the other hand, this is complicated by features such as multi-threading, so we'll have to see how things unfold.

# Conflicts: # src/ifcexpressparser/implementation.py # src/ifcgeom/IfcGeomFunctions.cpp # src/ifcparse/Ifc4.cpp

Stinkfist0 · 2016-04-24T15:57:28Z

I see. Cache with runtime configurable memory usage budget could be a nice solution in the long run.

Stinkfist0 · 2016-04-25T08:08:50Z

Also, could test and profile using unordered_map instead of map for the Cache if it would provide better performance (although it might have higher memory footprint).

aothms · 2016-04-25T08:22:46Z

Also, could test and profile using unordered_map instead of map for the Cache if it would provide better performance (although it might have higher memory footprint).

That would be a good idea I think. The keys are basically unique size_ts already so the hash function can just be an identity function. The memory usage of std::map treenodes is also not negligible, not sure if it would yield a higher mem footprint.

Anyway, I did some plots for memory usage for several files and with several caching configurations. Horizontal axis is time in seconds, vertical memory usage in mb.

Results do not appear very consistent although trashing the cache after 100 iterations proves to be a reasonable middle ground. Maybe I will have a go with unordered_map and compare.

aothms · 2016-04-27T11:14:30Z

@Stinkfist0 Using unordered_map turns out to yield a major improvement gain in terms of memory usage and processing time. Thanks for this! I will see if adapting the maps in the IfcFile class [1] offers improvements as well. I already thought having the map-by-id in a vector if ids are contiguous to have constant lookup times, but I guess a hash-table works for both contiguous and non-contiguous instance names. Although ordering might still make sense there when enumerating instances?

[1] https://github.com/IfcOpenShell/IfcOpenShell/blob/master/src/ifcparse/IfcFile.h#L35

Stinkfist0 · 2016-04-28T11:15:04Z

Great to hear. The classic std::map look-up has always had O(log n) complexity AFAIK.

aothms · 2016-04-30T09:02:21Z

I seem to have mis-labeled my builds. I can no longer reproduce a performance gain using std::unordered_map. It seemed to make sense to me however: we observe longer running times as the cache grows, so introducing a map with constant time lookup should alleviate this problem. Doesn't seem to be the case and there is a weird spike in memory usage in one particular occasion. So I'm inclined to keep things a std::map. I didn't look into the implementation details, of which there might be more than in case of a red-black tree. Since the number of cache entries can be estimated, it might we worthwhile to set a bucket count upfront to prevent resizes and rehashes, but I don't want to invest too much time into this caching structure that can hopefully be made obsolete in subsequent releases.

Trying to find a sensible number for the interval between cache clears doesn't show much of a sweet spot. Even a single iteration is beneficial as apparently within a single representation there is the most re-use, which is imaginable. I think I am going to settle on 64 as I like power of 2s. Needless to say this is very dependent on the type of file, but at least this shows it is not very sensitive to the exact value, as long as the cache is cleared once in a while. So it can be somewhat arbitrary.

vipcxj · 2018-12-24T05:50:00Z

Why not provide a option to set the clear_interval of cache. I have a big model containing more than 5000 ifcproduct. And after generating the geom data, 90% geom data are repeated. The default clear_interval 64 is too small.

aothms · 2018-12-24T12:04:29Z

@vipcxj I think discussion is a bit outdated. There are smarter ways to do this caching as IfcOpenShell could detect how long data needs to be retained. Also there is going to be multi-threading at some point which will likely have a big impact on caching.

Why not provide a option to set the clear_interval of cache

If you want to work on this, I'd happily merge a pull request, shouldn't be hard to implement, but I think it's better if we direct our attention to more clever strategies.

aothms added 6 commits March 18, 2016 11:14

Use constant time parent enumeration lookup

0ceef8f

python2.7 compatibility on code generator

74e495c

Don't iterate past material indices in collada serializer

614148e

Compile-time options to manipulate caching behaviour

8ecc126

Cleanup triangulations after use

4a3a0bf

Directly re-use IfcMappedItem triangulated elements when not using wo…

59d2d22

…rld coordinates

aothms mentioned this pull request Mar 18, 2016

IfcConvert enhancements #42

Merged

Merge branch 'master' into performance_improvements

ee56aad

# Conflicts: # src/ifcconvert/ColladaSerializer.cpp # src/ifcconvert/ColladaSerializer.h # src/ifcgeom/IfcGeomIterator.h # src/ifcgeom/IfcGeomRenderStyles.h

aothms reviewed Mar 22, 2016
View reviewed changes

aothms added 2 commits March 27, 2016 21:17

Immediately emit to serializer and remove option to obtain ownership …

4c574df

…of objects

Disable geometry re-use with world coords

25a5135

Merge branch 'master' into performance_improvements

c6d5adf

# Conflicts: # src/ifcexpressparser/implementation.py # src/ifcgeom/IfcGeomFunctions.cpp # src/ifcparse/Ifc4.cpp

Set reasonable defaults for caching behavior

a1d5847

aothms merged commit 2f12955 into IfcOpenShell:master May 1, 2016

aothms mentioned this pull request Sep 26, 2016

build-all.sh generated larger executable and it cost much longer to convert to OBJ #137

Closed

Stinkfist0 mentioned this pull request Oct 13, 2016

IfcMappedItem optimizations when item's StyledByItem count != 0 #143

Open

saq1410 mentioned this pull request Nov 8, 2016

Performance optimisation. #158

Closed

Moult mentioned this pull request Aug 21, 2021

Optimising geometry iterator initialisation #1674

Closed

bimintelligence mentioned this pull request Jan 1, 2023

Brep solid from IfcFace #2669

Closed

toporivskiy mentioned this pull request May 22, 2023

Swood Design (SolidWorks) - IFC read error #3182

Closed

monemati mentioned this pull request Oct 28, 2024

ifcjs - fail to load project without TrueNorth / assigned units (Memory access out of bounds) #5646

Closed

Uh oh!

Uh oh!

Conversation

aothms commented Mar 18, 2016

Uh oh!

aothms Mar 22, 2016

Choose a reason for hiding this comment

Uh oh!

Stinkfist0 Mar 22, 2016

Choose a reason for hiding this comment

Uh oh!

Stinkfist0 Mar 22, 2016

Choose a reason for hiding this comment

Uh oh!

aothms commented Mar 27, 2016

Uh oh!

Stinkfist0 commented Mar 28, 2016

Uh oh!

Stinkfist0 commented Mar 30, 2016

Uh oh!

Stinkfist0 commented Apr 8, 2016

Uh oh!

Stinkfist0 commented Apr 8, 2016

Uh oh!

Stinkfist0 commented Apr 8, 2016

Uh oh!

aothms commented Apr 9, 2016

Uh oh!

Stinkfist0 commented Apr 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aothms commented Apr 24, 2016

Uh oh!

Stinkfist0 commented Apr 24, 2016

Uh oh!

Stinkfist0 commented Apr 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aothms commented Apr 25, 2016

Uh oh!

aothms commented Apr 27, 2016

Uh oh!

Stinkfist0 commented Apr 28, 2016

Uh oh!

aothms commented Apr 30, 2016

Uh oh!

vipcxj commented Dec 24, 2018

Uh oh!

aothms commented Dec 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Stinkfist0 commented Apr 24, 2016 •

edited

Loading

Stinkfist0 commented Apr 25, 2016 •

edited

Loading