Skip to content

Commit

Permalink
Add developer notes draft
Browse files Browse the repository at this point in the history
  • Loading branch information
pramsey committed Jun 5, 2020
1 parent 3f053ae commit 6e2a8b5
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 4 deletions.
73 changes: 73 additions & 0 deletions DEVELOPER-NOTES.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
## GEOS is a port of JTS

* The algorithms that form the core value of GEOS are developed in Java in the [JTS library](https://github.com/locationtech/jts/). C++ developers will find this annoying, but:

* This is just history, JTS was written first and GEOS was a slavish port.
* Being memory managed, JTS is an easier language to prototype in.
* Having various visual tooling, JTS is an easier platform to debug spatial algorithms in.
* Being Java, JTS has less language legacy than GEOS, which was originally ported when STL was still not part of the standard, and therefor reflects a mix of styles and eras.

* Ideally, new algorithms will be implemented in JTS and then ported to GEOS.

This comment has been minimized.

Copy link
@dbaston

dbaston Jan 23, 2021

Member

I think this makes sense for some portions of the code base. Globally? Too much clash between GEOS and JTS development models.

* Smaller performance optimizations in GEOS can travel back to JTS.

* Short circuits, indexes, other non-language optimizations, should be ticketed in JTS when they are added to GEOS.

### Follow JTS as Much as Possible

* Don't rename things! It makes it harder to port updates and fixes.

* Class names
* Method names
* Variable names
* Class members

* Yes, we know in your last job you were taught all member variables are prefixed with `m_`, but please don't.

This comment has been minimized.

Copy link
@dbaston

dbaston Jan 23, 2021

Member

One reason for doing this is that JTS frequently reuses member variable names within methods (because there is no ambiguity in Java, you can only access a member variable with this.var), whereas this creates a variable shadowing in C++.

This comment has been minimized.

Copy link
@pramsey

pramsey Jan 23, 2021

Author Member

I leave the global member re-naming to you :)

This comment has been minimized.

Copy link
@dbaston

dbaston Jan 23, 2021

Member

I wouldn't advocate for renaming everything, but when you must rename in C++ to avoid a name shadowing, you may as well do it in the C++ conventional way.




### Manage Lifecycles

* Frequently objects are only used local to a method and not returned to the caller.
* In such cases, avoid lifecycle issues entirely by **instantiating on the local stack**.

```java
MyObj foo = new MyObj("bar");
```

```c++
MyObj foo("bar");
```
* Long-lived members of objects that are passed around should be held using [std::unique_ptr<>](https://en.cppreference.com/w/cpp/memory/unique_ptr).
```java
private MyMember foo = new MyMember();
```

```c++
private:

std::unique_ptr<MyMember> foo;

public:

MyMember()
: foo(new MyMember())
{}
```
* You can pass pointers to the object to other methods using `std::unique_ptr<>.get()`.
### Avoid Many Small Heap Allocations
* Heap allocations (objects created using `new`) are more expensive than stack allocations, but they can show up in batchs in JTS in places where structures are built, like index trees, or graphs.
* To both lower the overhead of heap allocations, and to manage the life-cycle of the objects, we recommend storing small objects in an appropriate "double-ended queue", like [std::deque<>](https://en.cppreference.com/w/cpp/container/deque).
* The implementation of `edgegraph` is an example.
* The `edgegraph` consists of a structure of many `HalfEdge` objects (two for each edge!), created in the `EdgeGraph::createEdge()` method and stored in a `std::deque<>`.
* The `std::deque<>` provides two benefits:
* It lowers the number of heap allocations, because it allocates larger blocks of space to store multiple `HalfEdge` objects.
* It handles the lifecycle of the `HalfEdge` objects that make up the `EdgeGraph`, because when the `EdgeGraph` is deallocated, the `std::deque<>` and all its contents are also automatically deallocated.
4 changes: 0 additions & 4 deletions src/edgegraph/EdgeGraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,7 @@ namespace edgegraph { // geos.edgegraph
HalfEdge*
EdgeGraph::createEdge(const Coordinate& orig)
{
// TODO: Overhead of many heap allocations might be
// a problem. Replace EdgeGraph::edges with a
// pool of some kind?
edges.emplace_back(orig);

return &(edges.back());
}

Expand Down

0 comments on commit 6e2a8b5

Please sign in to comment.